Numbers up to 10^21 shouldn't be displayed exponentially (Fixes #2751) #3635
Conversation
| maxOutDigits = g_rgcchSig[radix]; | ||
| __analysis_assume(maxOutDigits > 0); | ||
|
|
||
| if (wExp2 < -60 || wExp2 > 60) | ||
| if (wExp10 < -21 || wExp10 > 21) |
There was a problem hiding this comment.
Exp2 is already calculated above. Could we use that for comparison instead of calculating expensive Exp10?
|
@obastemur I changed it to compare |
|
@xiaoyinl could you please update the output under |
|
Isn't the exponential form correct in that case? |
|
|
@xiaoyinl @MSLaguana test case should be updated to The reason is, we ask for |
|
@obastemur @MSLaguana Done. The remaining question is what to do if the number is > 1e21. @littledan says other engines never switch to exponential notation for radix not 10, regardless of how large it is. Do you want ChakraCore to adopt this behavior? |
|
/cc @bterlson |
|
At this point; if tests pass, squashing commits could be nice |
|
Tests pass. I'll adopt this and merge it in after it passed tests internally. I have added some follow-up tasks:
|
…ponentially (Fixes #2751) Merge pull request #3635 from xiaoyinl:num_exp The spec section 7.1.12.1 requires that if a number's base 10 logarithm is <= 21, then it shouldn't use exponential notation when it's converted to String. The original code checks if the base 2 logarithm of the number is <= 60. This fixes #2751.
… displayed exponentially (Fixes #2751) Merge pull request #3635 from xiaoyinl:num_exp The spec section 7.1.12.1 requires that if a number's base 10 logarithm is <= 21, then it shouldn't use exponential notation when it's converted to String. The original code checks if the base 2 logarithm of the number is <= 60. This fixes #2751.
, #3740) Merge pull request #3742 from xiaoyinl:never_use_exp Other engines (SpiderMonkey, JSC and V8) never use exponential display for num.toString(radix) if radix is not 10, no matter how large num is. This PR updates the code and the test cases to match that behavior. This PR fixes #3739 and fixes #3740. See also: #2751, #3635. /cc @dilijev @littledan @bterlson
…ot 10 (Fix #3739, #3740) Merge pull request #3742 from xiaoyinl:never_use_exp Other engines (SpiderMonkey, JSC and V8) never use exponential display for num.toString(radix) if radix is not 10, no matter how large num is. This PR updates the code and the test cases to match that behavior. This PR fixes #3739 and fixes #3740. See also: #2751, #3635. /cc @dilijev @littledan @bterlson
The spec section 7.1.12.1 requires that if a number's base 10 logarithm is <= 21, then it shouldn't use exponential notation when it's converted to String. The original code checks if the base 2 logarithm of the number is <= 60.
This fixes #2751.