[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 #226702|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




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

Please only add code changes that either make the code easier to read, faster, or actually fix something we can demonstrate is wrong with a test. Most of the code change here is not needed.

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

When there is an exception, the return value is ignored. So this should just say

    return JSValue();

We shouldn’t waste any code computing a value that will be ignored.

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

Same issue here. But also, since there is no code execution after this point, there is no reason to do an early exit when an exception occurred. We can just let the product be computed and then the caller will ignore it. So this code should be deleted.

> Source/JavaScriptCore/runtime/MathObject.cpp:301
> +    uint32_t intMax = std::numeric_limits<int>::max();
> +    uint64_t product = left * right;
> +    if (product > intMax)
> +        return JSValue::encode(jsNumber((int32_t)(product-mathPow(2, 32))));
> +    return JSValue::encode(jsNumber((int32_t)product));

This code is written strangely. Why are we getting the maximum of "int" when the rest of this function is written in terms of types like int32_t? But also, it’s making a big mistake. The whole point of how imul is specified is that it defines behavior that can be efficiently implemented in C. The old code should have worked. Please try just changing the types from int32_t to uint32_t without adding this explicit overflow logic. That should fix the function. If it doesn’t fix the function, please provide a test case showing that it’s wrong.

To word this another way, I don’t see any added test case that covers this overflow issue fixed here, and I suspect this is fixing something that is not broken.

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