Draft: Replace MPFR with GMP-only solution #161
Conversation
|
Jenkins, ok to test. |
|
I am in favor of this change in general (less dependencies is always better regardless of the license issue). And if we preserve the same functionality, I don't think we need the build time option. |
|
Investigating the test failures in CI now. |
|
|
||
| static bool multiply_size_by_unit (mpfr_t size, char *unit_str) { | ||
| /* Helper function to convert mpf_t to mpz_t with round-toward-zero (truncate decimal) */ | ||
| static void mpz_set_f_round_zero(mpz_t rop, const mpf_t op) { |
There was a problem hiding this comment.
Doesn't libgmp already provide mpz_set_f which does this exact thing? https://gmplib.org/manual/Assigning-Integers
Relying on the formatting and parsing seems hacky.
There was a problem hiding this comment.
It's because libgmp's mpz_set_f doesn't handle the decimal truncation properly (edit: I guess I should say, it doesn't handle truncation the way we would like it to here. it's intended behaviour for libgmp to truncate the binary representation). As noted in the commit introducing mpfr, mpz_set_f(0.1 * 1000) is 99.
Agree that it's a bit hacky, though.
There was a problem hiding this comment.
Are you sure the string based method is different? Is there a test case that fails with mpz_set_f? It seems to me the issue with the 0.1 * 1000 case is not the truncation but the result of the multiplication itself.
There was a problem hiding this comment.
.....you're right. Damn, back to the drawing board. Thank you for the careful review. I just assumed the test cases had coverage for this without verifying, and they definitely don't.
MPFR is GPLv3 licensed, which is inconvenient for embedded projects. This commit removes the MPFR dependency and replaces it with a GMP-only solution. The issue: mpz_set_f() truncates binary representation, causing e.g. 0.1 * 1000 to become 99 instead of 100. We need round-towards-zero logic. To deal with this we add a new helper that converts mpf_t to decimal string, truncates at decimal point, then parses back to mpz_t. This provides correct decimal rounding equivalent to MPFR's mpfr_get_z() with MPFR_RNDZ. All tests pass with the GMP-only implementation. Signed-off-by: Jon Oster <jon.i.oster@gmail.com>
|
Thanks to @PlasmaPower for the review, this isn't mergeable and the approach is wrong. I'm going to go back to the drawing board and look for another solution; I'll make sure to add appropriate test cases to the test suite next time. |
|
Could rationals be used instead of floats for this? They seem like a better fit. |
|
Yes! That's exactly what I'm going for now. I did some fuzzing, and MPFR actually also gives incorrect results (ex: 0.0021209044 TB is 2120904399 bytes). The errors are about 10x less frequent than with libgmp, but I think the only way to get universally accurate results is with rationals. Will come back and reopen this when I've got something working. |
|
Ok, fix is done. As promised, I also added tests that will fail either with the MPFR implementation or the GMP-only implementation. Opening a new PR #163 because it seems I don't have the permissions to reopen this one. |
3 participants
See #160 for the reasoning. Happy to accept change requests, or change the patch so that it's a build-time option whether to use MPFR or not.