handle exponential floats #16
Conversation
|
Good catch! However, I don't think we can regex evaluate all numbers to cover the scientific notation case. Is there another way to distinguish them? There should be a cut-off where it is represented with those notions. However, I'm not sure that is platform agnostic. Regardless I think we'll have to find another approach for them as the regex at this very hot path is not really feasible I think. |
|
I haven't done any benchmarks, but i think it's valid concern... According to this reference
JS numbers seems to be platform agnostic, but i wouldn't rely on internal Number implementation because it may change. There is So using something kinda |
| it('floatExponentialOne', function() { | ||
| testPackUnpackHomogeneousArray(-1.552345411e+123, ONE, 2, true); | ||
| }); | ||
|
|
||
| it('floatExponentialMany', function() { | ||
| testPackUnpackHomogeneousArray(-1.552345411e+123, MANY, 2, true); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Another couple of test cases that would be worth adding is like 1e1 and 1e-1 for numbers that would generally not be expressed with this notation, but still could be.
There was a problem hiding this comment.
Makes sense. Added them in test/full/number.ts
src/compressor/number.ts
Outdated
| if(obj % 1 === 0) { | ||
| if(Number.isInteger(obj) && obj.toString() !== obj.toExponential()) { |
There was a problem hiding this comment.
I don't think you have to change the existing condition,
obj % 1 === 0 && obj.toString() !== obj.toExponential()
Should also work I think?
There was a problem hiding this comment.
Thought inner implementation should be more performant... But i see that according to docs it may return wrong result with high precision. Maybe you're right. Will revert this
There was a problem hiding this comment.
Thought inner implementation should be more performant... But i see that according to docs it may return wrong result with high precision. Maybe you're right. Will revert this
Yeah could be, hard to tell unless benchmarked. I’m mostly thinking just for compatibility
|
added few optimizations |
|
any more concerns here? |
Fix for #15
Hope it won't affect performance significantly