[Webkit-unassigned] [Bug 129884] Make Math.imul ES6 conform

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #2 from Darin Adler <darin at apple.com>  2014-03-07 17:54:55 PST ---
(From update of attachment 226129)
View in context: https://bugs.webkit.org/attachment.cgi?id=226129&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        Make Math.imul ES6 conform

This patch does not come along with sufficient test coverage for the exception code. We know that because there are bugs in it that I saw by code inspection, but no tests failed.

Also, this patch adds no tests at all. That violates the WebKit project policy. When fixing a bug we include a test that shows the bug and that it is now fixed.

> Source/JavaScriptCore/runtime/MathObject.cpp:289
>  EncodedJSValue JSC_HOST_CALL mathProtoFuncIMul(ExecState* exec)

Generally unclear what the algorithm here is trying to do. May need some more comments. Definitely need a lot more test coverage or we cannot be confident this is correct.

> Source/JavaScriptCore/runtime/MathObject.cpp:293
> +        JSValue::encode(jsNumber(left));

Two things wrong here:

1) Missing a "return" so this doesn’t do what you think it does.
2) There is no reason to do any work to return a particular value when there is an exception. The value will be ignored. I suggest returning JSValue(), which is probably the most efficient.

> Source/JavaScriptCore/runtime/MathObject.cpp:296
> +    if (exec->hadException())
> +        JSValue::encode(jsNumber(right));

Same problem again.

> Source/JavaScriptCore/runtime/MathObject.cpp:297
> +    uint32_t raised = mathPow(2, 32);

This doesn’t do what you think it does. The value of raised here will be 0, since 2^32 does not fit in a uint32_t.

> Source/JavaScriptCore/runtime/MathObject.cpp:298
> +    int32_t product = (int32_t)((left * right)%raised);

Incorrect formatting here (need spaces around the % operator). Incorrect style, using C-style casts. Likely also that this does not handle overflow properly. Need test cases that involve overflow and ideally test cases that almost overflow, but not quite. This code likely doesn’t work very well.

> Source/JavaScriptCore/runtime/MathObject.cpp:299
> +    if (product >= mathPow(2, 31))

I don’t think the compiler constant-folds the mathPow expression so this is unnecessarily inefficient. I personally would write:

    if (product >= (1U << 31))

Or something along those lines. Maybe better to use a named constant.

> Source/JavaScriptCore/runtime/MathObject.cpp:300
> +        return JSValue::encode(jsNumber(product-raised));

Need spaces around the "-" sign.

Also, I am pretty sure that overflow of uint32_t will produce these results without explicit special cases. Once you have a test in place that covers enough edge cases we can do some experiments with that.

Long term we are going to want to build an efficient version of this into the compiler, so the test cases are actually more important than the C++ code.

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