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

[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 226129: Patch v1
https://bugs.webkit.org/attachment.cgi?id=226129&action=review

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