fromFixnum(): fix for when argument fixnum is less than about 1e-7, brownplt/code.pyret.org#556 #1810
Conversation
|
The added test that |
| @@ -2036,6 +2055,8 @@ define("pyret-base/js/js-numbers", function() { | |||
|
|
|||
| var scientificPattern = new RegExp("^([+-]?\\d*\\.?\\d*)[Ee]([+]?\\d+)$"); | |||
|
|
|||
| var genScientificPattern = new RegExp("^([+-]?\\d*\\.?\\d*)[Ee]([+-]?\\d+)$"); | |||
There was a problem hiding this comment.
Is scientificPattern used elsewhere? Is it wrong when used elsewhere?
There was a problem hiding this comment.
scientificPattern, which doesn't allow a negative exponent, is used by expandExponent(), which is used by makeBignum(). I don't know why a negative exponent is explicitly disallowed. It's true that makeBignum(), since it makes "big" integers, doesn't need to handle negative exponents.
For fromFixnum(), we definitely need to handle negative exponents. The bug for which this fix was made was about misreading numbers like 5e-19 from an Excel sheet. So the new regexp pattern had to be introduced.
src/js/base/js-numbers.js
Outdated
| factor1 = exponentValue; | ||
| } | ||
| } | ||
| match = stringRep.match(/^(.*)\.(.*)$/); |
There was a problem hiding this comment.
Can we call factor1 something like exponentFactor instead? That would have helped me a lot to understand why it's initialized to 1.
There was a problem hiding this comment.
It would have also helped me to read the code if the match variable wasn't re-used.
src/js/base/js-numbers.js
Outdated
| } | ||
| } | ||
| match = stringRep.match(/^(.*)\.(.*)$/); | ||
| var factor2; |
There was a problem hiding this comment.
Similarly could this be called baseFactor?
src/js/base/js-numbers.js
Outdated
| } else { | ||
| return Rational.makeInstance(x, 1, errbacks); | ||
| factor2 = Rational.makeInstance(Number(stringRep), 1, errbacks); |
There was a problem hiding this comment.
Can you help me understand this change? What's an example where using x would have been wrong? I can't think of one, but maybe I'm not thinking through the ranges correctly.
There was a problem hiding this comment.
I had not updated x to be the "shorter" (i.e. sans exponent) number and hence had to use Number(stringRep).
In the most recent push, I've updated x and so can use Rational.makeInstance(x, 1, ...).
There was a problem hiding this comment.
Also, I found the older code dealing with the decimal point a bit opaque, so I've taken the liberty of using more expressive variable names and added comments to aid a future traveler. Hope that's ok.
|
I just spent a little more time reading js-numbers. Can we explain how this function differs in purpose from This PR is looking more and more like the pyret-lang/src/js/base/js-numbers.js Line 2057 in 175b934 In fact, Should we be re-using that logic? |
No description provided.