[webkit-reviews] review denied: [Bug 129486] Implement Math.hypot : [Attachment 225476] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Tibor Mészáros
<tmeszaros.u-szeged at partner.samsung.com>'s request for review:
Bug 129486: Implement Math.hypot
https://bugs.webkit.org/show_bug.cgi?id=129486

Attachment 225476: Patch
https://bugs.webkit.org/attachment.cgi?id=225476&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225476&action=review


There is insufficient test coverage for special case arguments in combinations
orders (0, NaN) (NAN, 0), and for exception and side effect behavior of the
function’s argument evaluation (valueOf functions on the arguments passed in).
Existing math functions don’t have that level of exception/side effect test
coverage, but they should.

> Source/JavaScriptCore/runtime/MathObject.cpp:186
> +    double* args = new double[argsCount];

Doing heap allocation here is bad for a variety of reasons:

1) This patch doesn’t include a delete, so it leaks.
2) It’s costly to do a heap allocation every time we evaluate this function,
and pointless for small argument counts.
3) This uses new, but we prefer to use fastNew, which uses a more efficient
version of malloc on some WebKit configurations.

If we really do need to copy all the arguments into a vector, then we should
use WebKit’s Vector. It’s something like this:

    Vector<double, 16> args(argsCount);

Then we’ll only do heap allocation if the number of arguments is more than 16.

> Source/JavaScriptCore/runtime/MathObject.cpp:189
> +    for (unsigned k = 0; k < argsCount; ++k) {

Why "k" when we use "-" everywhere else?

> Source/JavaScriptCore/runtime/MathObject.cpp:192
> +	   if (argsCount == 1 && (arg == -0 || arg == +0))
> +	       return JSValue::encode(jsNumber(0));

For the number 0 we are returning immediately, but for infinity and NaN we are
waiting until the end. Is that right? I don’t see test coverage for this
difference.

Are you sure that "arg == -0 || arg == +0" does what you think it does in
C/IEEE numerics? I’m pretty sure that both sides of the "||" will be true for
either type of zero.

> Source/JavaScriptCore/runtime/MathObject.cpp:194
> +	   if (std::isinf(arg))
> +	       inf = true;

Do we really need a special case for infinity? Does the algorithm and IEEE math
give us the right value without a special case? If so, we should just let it do
that.

> Source/JavaScriptCore/runtime/MathObject.cpp:196
> +	   else if (std::isnan(arg))
> +	       nan = true;

Do we really need a special case for NaN? Does the algorithm and IEEE math give
us the right value without a special case? If so, we should just let it do
that.

> Source/JavaScriptCore/runtime/MathObject.cpp:207
> +	   return JSValue::encode(jsDoubleNumber(QNaN));

This should be using jsNaN(), not jsDoubleNumber().

> Source/JavaScriptCore/runtime/MathObject.cpp:210
> +    if (max)
> +	   max = 1;

What does this mean? We looped through all the arguments just to compute max,
and then we convert it to either 0 or 1? Looks wrong to me.

> Source/JavaScriptCore/runtime/MathObject.cpp:212
> +    double comp = 0;

Do we really need to use the non-word "comp" for this argument? Is this a term
of art that needs to be abbreviated?

> Source/JavaScriptCore/runtime/MathObject.cpp:216
> +	   double prelim = sum + summand;

Same question about "prelim".