search

Agoric / nat

Ensures that a number is within the natural numbers (0, 1, 2...) or throws a RangeError
Apache License 2.0
5 stars 4 forks source link

Simplify to use Number.isSafeInteger() #8

Closed katelynsills closed 5 years ago

katelynsills commented 5 years ago

Description

This PR simplifies Nat to use Number.isSafeInteger(). In the specification, "when the Number.isSafeInteger is called with one argument number, the following steps are taken:

If Type(number) is not Number, return false. If number is NaN, +∞, or −∞, return false. Let integer be ToInteger(number). If integer is not equal to number, return false. If abs(integer) ≤ 2**53−1, return true. Otherwise, return false."

Thus, we can remove:

  1. The typeof check (line 23)
  2. The NaN check (line 33)
  3. The integer check (line 39)
  4. The safe number check (line 42)

This leaves us with two checks. If allegedNum is not a safe integer or is negative, we throw a RangeError. Otherwise we return the number.

Considerations

By simplifying, we are removing the distinct RangeError messages. Do we want to do that?

Test

npm test

warner commented 5 years ago

I'm going to leave this up to @erights in case there's a subtlety I'm not aware of.. I know part of Nat is making sure that two Nats can add together without wraparound, and I don't know if the range of isSafeInteger enforces that precisely (OTOH you checked the docs, and it's likely that isSafeInteger was written/championed/approved by MarkM specifically to implement Nat). Do our tests exercise both sides of the cutoff?

But https://caniuse.com/#feat=es6-number says that isSafeInteger is present in all modern browsers (i.e. not IE), and from what I can tell it's in Node.js back to version 6, so it's probably safe to assume it's present everywhere we care about.

katelynsills commented 5 years ago

Sounds good. It does look like it was championed by Mark specifically. Our tests exercise both sides of the cutoff, but they don't attempt to do any addition. Maybe we want to add that?