On Thu, 2024-01-25 at 16:04 -0500, Antoni Boucher wrote: > Thanks for the review! > On Wed, 2024-01-24 at 13:10 -0500, David Malcolm wrote: > > On Thu, 2024-01-11 at 18:42 -0500, Antoni Boucher wrote: > > > Hi. > > > This patch fixes the bug 113343. > > > I'm wondering if there's a better solution than using mpfr. > > > The only other solution I found is real_from_string, but that > > > seems > > > overkill to convert the number to a string. > > > I could not find a better way to create a real value from a host > > > double. > > > > I took a look, and I don't see a better way; it seems weird to go > > through a string stage. Ideally there would be a > > real_from_host_double, but I don't see one.
Sorry for the delay in responding > > > > Is there a cross-platform way to directly access the representation > > of > > a host double? > > I have no idea. Neither do I, but presumably this patch is fixing a real problem (no pun intended)... > > > > > > If there's no solution, do we lose some precision by using mpfr? > > > Running Rust's core library tests, there was a difference of one > > > decimal, so I'm wondering if there's some lost precision, or if > > > it's > > > just because those tests don't work on m68k which was my test > > > target. > > > > Sorry, can you clarify what you mean by "a difference of one > > decimal" > > above? > > Let's say the Rust core tests expected the value "1.23456789", it > instead got the value "1.2345678" (e.g. without the last decimal). > Not sure if this is expected. > Everything works fine for x86-64; this just happened for m68k which > is > not well supported for now in Rust, so that might just be that the > test > doesn't work on this platform. > > > > > > Also, I'm not sure how to write a test this fix. Any ideas? > > > > I think we don't need cross-compilation-specific tests, we should > > just > > use and/or extend the existing coverage for > > gcc_jit_context_new_rvalue_from_double e.g. in test-constants.c and > > test-types.c > > > > We probably should have test coverage for "awkward" values; we > > already > > have coverage for DBL_MIN and DBL_MAX, but we don't yet have test > > coverage for: > > * quiet/signaling NaN > > * +ve/-ve inf > > * -ve zero ...and I'm guessing you've tested this code on all of the various configurations you're targeting. Assuming that, this patch is good for trunk. > > Is this something you would want for this patch? No, that's just for bonus points :) Thanks Dave
