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:
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).
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);
Shouldn't the threshold be closer to 1.1e-16 instead of 1e-14?
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 {
Should this be renamed? This no longer holds just trig constants. Maybe
FdlibmConstants instead?
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]).
Update to say log1p is also done? Or maybe just remove the comment about
sin, cos, tan?
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode427
third_party/fdlibm/fdlibm.js:427: const TWO_THIRD = kMath[37];
I'm curious on why you added these constants here for logp1. You didn't
do that for the trig functions. Is there some advantage here (or
disadvantage in the trig functions)?
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode437
third_party/fdlibm/fdlibm.js:437: var f = 0;
Perhaps initialize f to x here and remove the assignment in line 464.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode440
third_party/fdlibm/fdlibm.js:440: var u = 0;
Maybe intialize u to x here and remove the assignment in line 480.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode444
third_party/fdlibm/fdlibm.js:444: if (ax >= 0x3ff00000) {
I would add a comment that |x| >= 1. (Really hard to know that's what ax
= 0x3ff00000 means.)
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)) {
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.
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
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.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode465
third_party/fdlibm/fdlibm.js:465: hu = 1;
Line 438 initializes hu to 1, so this can probably be removed.
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;
Add comment to say this handles infinities and NaN.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode473
third_party/fdlibm/fdlibm.js:473: // x < 9.007199254740992e15
Maybe note that 9.007...e15 is actually exactly 2^53.
https://codereview.chromium.org/457643002/diff/1/third_party/fdlibm/fdlibm.js#newcode483
third_party/fdlibm/fdlibm.js:483: c = 0;
Since c is initialized to 0 in line 439, perhaps we can remove this.
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));
fdlibm said this is normalizing u/2, so probably should add that
comment.
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.
