search

paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 389 forks source link

fix for Math.round #325

Closed Yaffle closed 9 years ago

ljharb commented 9 years ago

Great, thanks for the explanation! If you can restore the original feature tests in the main shim file, and add a new, separate variable that represents your new feature tests, I'd be happy to merge this!

ljharb commented 9 years ago

@Yaffle why did you close this?

Yaffle commented 9 years ago

@ljharb , too many commits... I need to a add a comment message about buggy browsers also

ljharb commented 9 years ago

No need to clutter up the repo with a second PR, just rebase this one and force push :-)

Yaffle commented 9 years ago

@ljharb , I do not know how to rebase repo.

ljharb commented 9 years ago

@Yaffle While I suggest you research how and learn (it's an awesome and powerful tool), you can also git branch -m patch-2 tmp ; git checkout master ; git checkout -b patch-2 and then check out any changes you want from tmp, and force push patch-2 which will update the PR accordingly.

Yaffle commented 9 years ago

@ljharb thanks for help with git

Yaffle commented 9 years ago

If you are interested - I am trying to add tests to test262: https://github.com/tc39/test262/pull/219

ljharb commented 9 years ago

OK, if we could condense this down to 1 or 2 commits on top of latest master one last time, I'll merge it in ASAP :-) From your explanations, I can figure out how to name the constants later, so thanks for that!

edit: on second thought, I'll merge this manually.

Yaffle commented 8 years ago

@ljharb

You mean conceptually? The reason I put both is because I found engines where in fact they aren't the same.

No, I am not. The engine for which 0.5 - Number.EPSILON / 4 is not equal to 0.5 - Number.EPSILON / 3.99` (when Number.EPSILON is defined), probably, has much bigger problems with floating point arithmetic.

0.5 - Number.EPSILON / 4 * 1 <= 0.5 - Number.EPSILON / 3.99 <= 0.5 - Number.EPSILON / 4 * (4 / 3.99), BUT there should exist only next numbers: 0.5 - Number.EPSILON / 4 * 1 0.5 - Number.EPSILON / 4 * 2 And so this value SHOULD be equal to 0.5 - Number.EPSILON / 4 * 1. Well, that engine MAY have some other bug which affects this....

Currently, you are replacing Math.round with your implementation even in Chrome and Firefox, where it is not buggy, seems.

Yaffle commented 8 years ago

@ljharb ?

ljharb commented 8 years ago

@Yaffle I would be happy to accept a PR with failing tests, or improvements - I'm not sure I grasp exactly why there's an issue, and since the spec does not mandate any precision, I'm afraid I won't have time to get to it quickly (absent a PR).

Yaffle commented 8 years ago

@ljharb 1) you are replacing not buggy Math.round in Chrome; 2) you have strange code