-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
feat(stdlib): Number.parseFloat #1288
Conversation
436c35a
to
79a319e
Compare
ab8814c
to
b444707
Compare
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.
Read every line of code, understood like 20% but the tests look great. Most of my comments are about license attribution, links, and comment style.
stdlib/runtime/atof/slow.gr
Outdated
/// Parse the significant digits and biased, binary exponent of a float. | ||
/// | ||
/// This is a fallback algorithm that uses a big-integer representation | ||
/// of the float, and therefore is considerably slower than faster | ||
/// approximations. However, it will always determine how to round | ||
/// the significant digits to the nearest machine float, allowing | ||
/// use to handle near half-way cases. | ||
/// | ||
/// Near half-way cases are halfway between two consecutive machine floats. | ||
/// For example, the float `16777217.0` has a bitwise representation of | ||
/// `100000000000000000000000 1`. Rounding to a single-precision float, | ||
/// the trailing `1` is truncated. Using round-nearest, tie-even, any | ||
/// value above `16777217.0` must be rounded up to `16777218.0`, while | ||
/// any value before or equal to `16777217.0` must be rounded down | ||
/// to `16777216.0`. These near-halfway conversions therefore may require | ||
/// a large number of digits to unambiguously determine how to round. | ||
/// | ||
/// The algorithms described here are based on "Processing Long Numbers Quickly", | ||
/// available here: <https://arxiv.org/pdf/2101.11408.pdf#section.11>. |
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.
Nit. Drop rust style comments and use a grain block comment
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.
One minor comment. I admit a lot of the finer details went over my head also (i.e. lemire.gr
), but I am reasonably assured that the parsing logic itself and such is correct. It wouldn't hurt to have another team member look it over (cc @marcusroberts and @ospencer), but I'm comfortable approving.
stdlib/runtime/atof/decimal.gr
Outdated
let (==) = WasmI32.eq | ||
let (+) = WasmI32.add | ||
let (-) = WasmI32.sub | ||
// All of the following calls to `trim` are fine because: |
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.
@jozanza Maybe this comment should refer more precisely to where the call site is (rather than "the following")?
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 can't comment on the correctness from the code, but the tests look good.
So cool to see stuff like this written in Grain too!
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.
WIP
Trying to implement Lemire with a slow path fallback.
Using these as a reference primarily: