Change Float.nan from sNaN to qNaN#10899
Conversation
|
It could be nice to have a comment to explain the reason for the new choice, and maybe event to document it in the .mli? Are there floating-point people that we want to warn, and get feedback from? (I'm thinking of the Frama-C people in particular.) |
There is already a comment in There is also a comment about |
|
(Adding @silene to the discussion.) That's the paradigmatic PR where the code change is easy, but agreeing on the change and making sure it doesn't break anything is difficult! Elaborating on my comment in #10862
Suspiciously, this is the smallest unsigned 64-bit value that encodes a FP NaN, so this may explain the choice of this particular value... Another possible explanation is that it was the default quiet NaN of some processor at the time -- I suspect the Alpha. The convention "top payload bit = 1 for quiet NaNs, = 0 for signaling NaNs" was not standardized until IEEE 754-2008. Some 1990's architectures used the opposite convention, most notably MIPS and probably Alpha too. In 2001 at Inria there was no MIPS machine left, but we still used Alpha servers, so this may be an explanation. Enough history. Now the question is: if we put forward one specific NaN value, which is most useful, a quiet NaN or a signaling NaN? It's not easy to cause signaling NaNs to actually raise signals (there is no ISO C standard API to do this), and OCaml doesn't know how to turn these signals into exceptions, so signaling NaNs can be assumed not to signal. However, some C library functions treat signaling NaNs and quiet NaNs differently (an example is in #10862)... Of course we could add |
|
I don't have much to say on the topic, so I will just rehash some of the points above.
|
|
I agree with the comments by silene. I would like to add that keeping a sNaN in Float would be useful: it is typically the value one should choose to fill an uninitialized array of floats (sNaNs have been introduced for that purpose in IEEE 754). Deciding between a sNaN and a qNan is difficult because even the documentation associated with Float.nan is contradictory (the NaN which is the result of 0.0 /. 0.0 is necessarily a qNaN while a NaN that always yield a NaN result when passed to any function is necessarily a sNaN). In case of a change to a qNaN, there should be a way to spot locations in the code where an old nan is used, maybe by providing two new definitions (Float.{snan, qnan}) and by decorating nan's definition with [@@ocaml.deprecated "Use Float.snan or Float.qnan instead."] ? |
|
Thank you all for the comments. Would it be better to put @silene I changed @sebfuric I am afraid that adding a deprecation warning on |
I would be tempted to add |
Fixed. @silene I had to change the definition |
|
Ping :) |
|
I don't have a strong opinion. I feel that the tests are somehow flawed, since they rely on the sign of NaN. Expression Perhaps using |
5152d19 to
f81fc85
Compare
Fixed, thanks! The CI is green. |
|
@xavierleroy I've addressed all the comments. Can this be merged? |
|
Sorry, I was occupied with other things lately. At any rate, I'm not a fan of This justification is lost if Also, for debugging purposes, a user may wish to turn NaN generation into a signal, using non-portable C code ( So, I'd much prefer to define the quiet NaN as |
|
The payload does not include sign bit, so the sign argument should not be relevant as far as payloads only are to be considered (abs only changes the sign bit of a qNaN). |
done! |
[nan] returns [quiet_nan] Update comments
Ensure sign bit is positive
|
Sorry for leaving this PR hanging for so long. @Octachron and I agreed to not rush it in OCaml 5.0 and schedule it for 5.1. |
See #10862 (comment)