Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editorial: remove unhelpful and potentially misleading checks in Number::lessThan #3174

Closed
wants to merge 1 commit into from

Conversation

michaelficarra
Copy link
Member

As discussed in editor call. Noticed during review of #3169. These checks for zero don't provide anything in terms of readability. The other unnecessary checks of this form in Number::sameValue are helpful for comparison to Number::sameValueZero, and should remain.

Aside: I would maybe also add an emu-note to this AO that says something like "+∞𝔽 is not strictly less than any Number; no Number is strictly less than -∞𝔽" since inferring this from the sequence of steps handling infinities is harder than reading such a declaration.

@bakkot
Copy link
Contributor

bakkot commented Sep 14, 2023

I thought the outcome of editor call was that these are helpful?

In particular, the only reason you could possibly be looking at this AO is because you want to know how edge cases are handled, and comparing -0 to +0 is definitely an edge case.

@michaelficarra
Copy link
Member Author

It's really clear that zeroes are neither NaNs nor Infinities so they fall into the to-real case which has obvious semantics. I'll add back to editor call.

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Sep 14, 2023
@ljharb
Copy link
Member

ljharb commented Sep 14, 2023

My recollection of the discussion was indeed that they're helpful and should be kept, despite being redundant.

@michaelficarra
Copy link
Member Author

The plan was that I would look at other similar guards in other AOs and compare the usefulness, and open this PR if I felt that this case provided less value than the other cases, which I feel it does.

@ljharb
Copy link
Member

ljharb commented Sep 14, 2023

"both zero, with different signs" is a case that does not to me have obvious semantics in the "reals" scenario, and it's very helpful to me to have them treated specially, even though they technically don't need to be.

@michaelficarra
Copy link
Member Author

"both zero, with different signs" is a case that does not to me have obvious semantics in the "reals" scenario, and it's very helpful to me to have them treated specially, even though they technically don't need to be.

I don't believe you. Both float zeroes convert to the single unsigned real zero. 0 < 0 is clearly false. Which of those is non-obvious?

@ljharb
Copy link
Member

ljharb commented Sep 14, 2023

It is wildly unintuitive to me that -0 < 0 is not true, so I have to check this quite frequently. I don't care about the "reals" part, I think in JS numbers when I'm writing JS.

@michaelficarra
Copy link
Member Author

Yes, but that's not what we're talking about here. We're talking about someone reading the spec steps of Number::lessThan to answer the question of what -0 < 0 results in. For such a reader, they will find their answer without confusion, even without these steps. That is why I think the steps should be removed. They add noise.

@bakkot
Copy link
Contributor

bakkot commented Sep 14, 2023

You have to do a lot more work to find the answer after these changes.

@syg
Copy link
Contributor

syg commented Sep 14, 2023

It is also my recollection that the outcome of the editor call we keep these.

@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Sep 19, 2023
@bakkot bakkot closed this Oct 22, 2023
@ljharb ljharb deleted the number-less-than-zero-checks branch October 22, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants