Bug 164464 – Math.min()/Math.max() with no arguments is lowered incorrectly in the BytecodeParser

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

They return NaN. However,
Math.min() === Infinity
Math.max() === -Infinity

[Saam Barati]

<rdar://problem/29131452>

[Saam Barati]

Created attachment 294031 [details]
patch

[WebKit Commit Bot]

Attachment 294031 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/stress/math-max-min-no-arguments.js:7:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/math-max-min-no-arguments.js:11:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/math-max-min-no-arguments.js:15:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/math-max-min-no-arguments.js:16:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/math-max-min-no-arguments.js:17:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/math-max-min-no-arguments.js:18:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 8 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 294031 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294031&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2119
> +        set(VirtualRegister(resultOperand), addToGraph(JSConstant, OpInfo(m_graph.freeze(jsNumber(result)))));

jsDoubleNumber is more efficient than jsNumber for this use, so it should be jsDoubleNumber here; saw this pattern in places in the runtime that deal with infinity.

[Mark Lam]

Comment on attachment 294031 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294031&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2116
>      if (argumentCountIncludingThis == 1) { // Math.min()

This "// Math.min()" is now stale.  I think you should remove it.

[Saam Barati]

(In reply to comment #5)
> Comment on attachment 294031 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294031&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2116
> >      if (argumentCountIncludingThis == 1) { // Math.min()
> 
> This "// Math.min()" is now stale.  I think you should remove it.

I think the comment was there just to be illustrative about the number of arguments to the function. That said, I don't think the comment is helpful and I'll remove it. The variable name we're comparing against is already illustrative enough.

[Saam Barati]

(In reply to comment #4)
> Comment on attachment 294031 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294031&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2119
> > +        set(VirtualRegister(resultOperand), addToGraph(JSConstant, OpInfo(m_graph.freeze(jsNumber(result)))));
> 
> jsDoubleNumber is more efficient than jsNumber for this use, so it should be
> jsDoubleNumber here; saw this pattern in places in the runtime that deal
> with infinity.

Sounds good. I'll switch to this.

[Saam Barati]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 294031 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=294031&action=review
> > 
> > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2116
> > >      if (argumentCountIncludingThis == 1) { // Math.min()
> > 
> > This "// Math.min()" is now stale.  I think you should remove it.
> 
> I think the comment was there just to be illustrative about the number of
> arguments to the function. That said, I don't think the comment is helpful
> and I'll remove it. The variable name we're comparing against is already
> illustrative enough.

I'll remove all forms of these comments in the function. I don't think they're helpful when the parameter name tells you exactly what's going on.

[Saam Barati]

Created attachment 294036 [details]
patch for landing

I'll wait until EWS is back up and running before landing the patch

[Saam Barati]

Created attachment 294037 [details]
patch for landing

fix style issues

[WebKit Commit Bot]

Comment on attachment 294037 [details]
patch for landing

Clearing flags on attachment: 294037

Committed r208496: <http://trac.webkit.org/changeset/208496>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.