[webkit-reviews] review denied: [Bug 129884] Make Math.imul ES6 conform : [Attachment 226702] Patch v3

[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 129884: Make Math.imul ES6 conform
https://bugs.webkit.org/show_bug.cgi?id=129884

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.