-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: clarify string representation of numbers with radix ≠ 10 #2854
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than comment but I want to wait for @syg's approval as well.
945a61f
to
b6def86
Compare
@michaelficarra %Bigint.prototype.toString% uses the same “should be a generalization” text and points at BigInt::toString. As far as I can tell the algorithm-in-practice aligns with Number-when-not-base-10. Would it make sense (and would it be desirable) for them to point to a common algorithm that’s agnostic to the specific numeric language type? (This PR already stands on its own with plenty of value, so I’m not suggesting it should address anything related to %BigInt.prototype.toString% — just curious about how that could end up looking.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the improved generality and precision, and this does a decent job of containing the decimal-specific scale weirdness. That said, I do wonder if the latter could be achieved even better, e.g.
1. If _radix_ ≠ 10 or _n_ is in the inclusive interval from -5 to 21, then
1. If _n_ ≥ _k_, return the string-concatenation of…
1. If _n_ > 0, return the string-concatenation of…
1. Else,
1. Assert: _n_ ≤ 0.
1. Return the string-concatenation of…
1. Assert: _radix_ is 10.
1. …
@gibson042 Thanks for the suggestions. I took all of them. Please re-review to check my work. @bathos Agree, we could do something similar about |
Thanks @jmdyck. Done. |
Fixes #2847 by adding a
radix
parameter toNumber::toString
./cc @bathos