LibJS: Add & use non-BigInt overload of round_number_to_increment() #10385
Conversation
linusg
force-pushed
the
libjs-temporal-double-roundnumbertoincrement
branch
2 times, most recently
from
28fad04
to
278d912
Compare
| VERIFY(rounding_mode == "ceil"sv || rounding_mode == "floor"sv || rounding_mode == "trunc"sv || rounding_mode == "halfExpand"sv); | ||
|
|
||
| // 3. Let quotient be x / increment. | ||
| auto quotient = x / (double)increment; |
There was a problem hiding this comment.
This cast to double can be out of range, not sure if increment can have the full i64 range here though.
There was a problem hiding this comment.
Fun fact: this is the firs time ever I look at V8's in progress Temporal implementation, and they do literally exactly the same:
(don't know how to link to it, gerrit is terrible)
So, that should answer both of your questions - there are plenty of cases where the number range is way less than what this can handle, and a i64 -> double cast seems fine as well.
|
I went through the spec, seems like the absolute largest value for |
Sounds good to me then, might want to add a small comment, but up to you |
Unlike the spec we chose BigInt for the input and output types here as it was being used with ℝ(ns), ns being of type BigInt, in one place and a conversion to double would not be safe. Since in many places we'll have double input values, let's add a double overload of this function to avoid awkward conversions and expensive allocations.
linusg
force-pushed
the
libjs-temporal-double-roundnumbertoincrement
branch
from
278d912
to
2781119
Compare
In preparation of this monstrosity, and because I'm not going to do double -> bigint -> double a dozen times: