[webkit-reviews] review denied: [Bug 104147] Math.{max, min}() must not return after first NaN value : [Attachment 223974] Patch v2

[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 104147: Math.{max, min}() must not return after first NaN value
https://bugs.webkit.org/show_bug.cgi?id=104147

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

------- Additional Comments from Darin Adler <darin at apple.com>
The tests added look like fine minimal coverage of this change, but we also
need to update the expected results in LayoutTests/js/math-expected.txt; please
upload a new patch with that.

I also think the code change is incorrect. It looks like this code will
continue to evaluate values even if ToNumber raises an exception. I don’t think
that’s right. I think that once a ToNumber raises an exception, we should stop
iterating through the rest of the arguments. The old code incorrectly had this
behavior any time there is a NaN involved, but the new code incorrectly
continues after an exception.

To test this we probably need test cases that involve side effects, not just
exceptions. Here is one example (a bit clumsily written) that I think will show
we have it wrong:

    sideEffect = 0;
    shouldThrow("Math.min({valueOf:function(){throw 'error1'}},
valueOf:function(){sideEffect = 1}})", "error1")
    shouldBe('sideEffect', '0');

I believe sideEffect will be 1.