https://codereview.chromium.org/457643002/diff/1/test/mjsunit/es6/math-log1p.js
File test/mjsunit/es6/math-log1p.js (right):
https://codereview.chromium.org/457643002/diff/1/test/mjsunit/es6/math-log1p.js#newcode20
test/mjsunit/es6/math-log1p.js:20:
On 2014/08/08 17:32:16, Raymond Toy wrote:
Although not part of the CL, I noticed this. I think this test should
avoid
testing down to 0.1, where log(x+1) and log1p(x) are supposed to be
different.
Based on the comments from fdlibm log1p(x) = log(1+x) when x > 2^53.
Then your
delta should be zero, or at least closer to 1.1e-16 (float epsilon).
Done.
https://codereview.chromium.org/457643002/diff/1/test/mjsunit/es6/math-log1p.js#newcode38
test/mjsunit/es6/math-log1p.js:38: assertEqualsDelta(expected,
Math.log1p(x), expected * 1E-14);
On 2014/08/08 17:32:16, Raymond Toy wrote:
Shouldn't the threshold be closer to 1.1e-16 instead of 1e-14?
Done.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.h
File third_party/fdlibm/fdlibm.h (right):
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.h#newcode25
third_party/fdlibm/fdlibm.h:25: struct TrigonometricConstants {
On 2014/08/08 17:32:16, Raymond Toy wrote:
Should this be renamed? This no longer holds just trig constants.
Maybe
FdlibmConstants instead?
Renamed to MathConstants. Fdlibm is implied by namespace.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js
File third_party/fdlibm/fdlibm.js (right):
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode17
third_party/fdlibm/fdlibm.js:17: // sin, cos, and tan, by Raymond Toy
([email protected]).
On 2014/08/08 17:32:17, Raymond Toy wrote:
Update to say log1p is also done? Or maybe just remove the comment
about sin,
cos, tan?
Done.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode437
third_party/fdlibm/fdlibm.js:437: var f = 0;
On 2014/08/08 17:32:16, Raymond Toy wrote:
Perhaps initialize f to x here and remove the assignment in line 464.
Done.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode440
third_party/fdlibm/fdlibm.js:440: var u = 0;
On 2014/08/08 17:32:17, Raymond Toy wrote:
Maybe intialize u to x here and remove the assignment in line 480.
Done.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode444
third_party/fdlibm/fdlibm.js:444: if (ax >= 0x3ff00000) {
On 2014/08/08 17:32:17, Raymond Toy wrote:
I would add a comment that |x| >= 1. (Really hard to know that's what
ax >=
0x3ff00000 means.)
Done.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode453
third_party/fdlibm/fdlibm.js:453: if ((TWO54 + x > 0) && (ax <
0x3c900000)) {
On 2014/08/08 17:32:17, Raymond Toy wrote:
The original fdlibm code said two54+x > 0 was meant to raise the
inexact flag. I
don't think JS exposes this, so perhaps this test can be removed.
Done.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode454
third_party/fdlibm/fdlibm.js:454: // |x| < 2^-54, so just return x
On 2014/08/08 17:32:17, Raymond Toy wrote:
Probably should update the comment to say
// |x| < 2^-29. For |x| < 2^-54, we can return x. Otherwise use simple
2-term
Taylor series.
Done.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode465
third_party/fdlibm/fdlibm.js:465: hu = 1;
On 2014/08/08 17:32:17, Raymond Toy wrote:
Line 438 initializes hu to 1, so this can probably be removed.
Done.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode469
third_party/fdlibm/fdlibm.js:469: if (hx >= 0x7ff00000) return x + x;
On 2014/08/08 17:32:17, Raymond Toy wrote:
Add comment to say this handles infinities and NaN.
-Infinity is actually caught further up. This only handles Infinity and
NaN. Added comment.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode473
third_party/fdlibm/fdlibm.js:473: // x < 9.007199254740992e15
On 2014/08/08 17:32:18, Raymond Toy wrote:
Maybe note that 9.007...e15 is actually exactly 2^53.
Done.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode483
third_party/fdlibm/fdlibm.js:483: c = 0;
On 2014/08/08 17:32:17, Raymond Toy wrote:
Since c is initialized to 0 in line 439, perhaps we can remove this.
Done.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode491
third_party/fdlibm/fdlibm.js:491: u = %_ConstructDouble(hu | 0x3fe00000,
%_DoubleLo(u));
On 2014/08/08 17:32:17, Raymond Toy wrote:
fdlibm said this is normalizing u/2, so probably should add that
comment.
Done.
https://codereview.chromium.org/457643002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
