ecma262 icon indicating copy to clipboard operation
ecma262 copied to clipboard

Normative: Allow NaN values to be *optionally* canoncalized

Open littledan opened this issue 9 years ago • 10 comments

This patch legalizes V8's occasional canonicalization of NaNs by changing SetValueInBuffer to either a particular value for the implementation-distinguishable NaN value or an implementation-defined canonical value.

This semantic change reached consensus at the November 2016 TC39 meeting.

Closes https://github.com/tc39/ecma262/issues/635

This patch still needs the associated test262 update. The tests that are affected include these, but maybe more:

built-ins/Object/internals/DefineOwnProperty/nan-equivalence
built-ins/TypedArray/prototype/fill/fill-values-conversion-operations-consistent-nan
built-ins/TypedArrays/internals/DefineOwnProperty/conversion-operation-consistent-nan
built-ins/TypedArrays/internals/Set/conversion-operation-consistent-nan

littledan avatar Dec 28 '16 16:12 littledan

cc @erights @allenwb

littledan avatar Dec 28 '16 16:12 littledan

@allenwb does this seem fine to you? (Just checking as I think you were involved in the discussions closely at the last meeting).

bterlson avatar Dec 29 '16 18:12 bterlson

What do we need from here to decide whether to land this patch?

@jfbastien has clarified that, even if this patch lands, we won't really have a full description of what happens with NaN values in practice. This is because processors sometimes change the NaN bit pattern later, e.g., in MOV instructions, in a way that JS implementations which don't canonicalize are probably not guarding against.

Even as I'd personally prefer that we leave the NaN bit pattern completely unspecified, I think this patch still brings us a little bit closer to reality and would be helpful to land.

littledan avatar Mar 05 '18 10:03 littledan

Thanks for the reviews everyone!

littledan avatar Mar 08 '18 21:03 littledan

@littledan would you mind rebasing this PR, and summarizing the current status? I believe it has consensus, and i'm not sure if it still needs tests.

@rwaldron can you rereview after the PR is rebased?

ljharb avatar Apr 26 '19 00:04 ljharb

Well, I would not feel comfortable making the change suggested above, to give more guarantees to Float32, given that this is unlikely to correspond to actual implementations. I would rather move this patch to removing any guarantees about how floats are canonicalized, given the information from @jfbastien, about how this is the implementation reality and is unlikely to change. @waldemarhorwat encouraged this path as well. @erights, would you be OK with this change?

littledan avatar Apr 26 '19 14:04 littledan

Yes, this change looks good to me.

erights avatar Apr 26 '19 15:04 erights

@erights To be concrete, I'm talking about, not this PR, but instead a PR to say that there's no stability for NaN (since this is the implementation reality).

littledan avatar Apr 26 '19 17:04 littledan

@erights To be concrete, I'm talking about, not this PR, but instead a PR to say that there's no stability for NaN (since this is the implementation reality).

Thanks for clarifying. I did not know that. Link to other PR?

erights avatar Apr 27 '19 02:04 erights