[Webkit-unassigned] [Bug 129486] Implement Math.hypot

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=129486


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #225476|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2014-03-01 15:53:38 PST ---
(From update of attachment 225476)
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".

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.