Skip to content

Conversation

@bryant
Copy link

@bryant bryant commented Jun 5, 2017

  • self doesn't need to be mut when get_str
  • impl Display and Debug for Mpf

@andersk
Copy link

andersk commented Jun 5, 2017

Don’t you think the Debug/Display interface ought to produce a string with a decimal point, even though the internal get_str function omits it?

/// Due to the way `Mpf::get_str` works, the output contains an implicit
/// radix point to the left of the first digit. `3.14`, for instance, will
/// print as `314e1` which means `0.314e1`.
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to duplicate code; you can just delegate to fmt::Display::fmt(&self, f).

src/mpf.rs Outdated
let out;
unsafe{
out = CString::from_raw(__gmpf_get_str(c_str.into_raw(), exp, base, n_digits, &mut self.mpf));
out = CString::from_raw(__gmpf_get_str(c_str.into_raw(), exp, base, n_digits, &self.mpf));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your tests have exposed the (pre-existing) bug that this is a blatant buffer overflow of c_str. The mpf_get_str documentation says “The area at str has to have space for the largest possible number represented by a s1n long limb array, plus one extra character.”

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not seeing a quick and easy entry point to calculate the number of digits. the easiest way would be to let gmp_allocate_func do the work.

bryant added 2 commits June 6, 2017 21:00
from https://gmplib.org/manual/Converting-Floats.html : `Function: char
* mpf_get_str (char *str, mp_exp_t *expptr, int base, size_t n_digits,
const mpf_t op)`
@bryant
Copy link
Author

bryant commented Jun 7, 2017

will rebase after #25 is merged.

src/mpf.rs Outdated
impl fmt::Display for Mpf {
/// Due to the way `Mpf::get_str` works, the output contains an implicit
/// radix point to the left of the first digit. `3.14`, for instance, will
/// print as `314e1` which means `0.314e1`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment needs to be updated from below.

this incorrectly created an uninitialized Vec, and using uninitialized
isn't worth it anyway.
@bryant bryant force-pushed the mpf-fixups branch 2 times, most recently from 4e3c307 to f248285 Compare June 10, 2017 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants