Bug 164464 - Math.min()/Math.max() with no arguments is lowered incorrectly in the BytecodeParser
Summary: Math.min()/Math.max() with no arguments is lowered incorrectly in the Bytecod...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-11-06 11:47 PST by Saam Barati
Modified: 2016-11-09 15:26 PST (History)
13 users (show)

See Also:


Attachments
patch (3.09 KB, patch)
2016-11-06 12:00 PST, Saam Barati
darin: review+
Details | Formatted Diff | Diff
patch for landing (3.81 KB, patch)
2016-11-06 14:26 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (3.84 KB, patch)
2016-11-06 14:29 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-11-06 11:47:30 PST
They return NaN. However,
Math.min() === Infinity
Math.max() === -Infinity
Comment 1 Saam Barati 2016-11-06 11:48:30 PST
<rdar://problem/29131452>
Comment 2 Saam Barati 2016-11-06 12:00:06 PST
Created attachment 294031 [details]
patch
Comment 3 WebKit Commit Bot 2016-11-06 12:01:03 PST
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.
Comment 4 Darin Adler 2016-11-06 12:07:47 PST
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.
Comment 5 Mark Lam 2016-11-06 12:34:16 PST
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.
Comment 6 Saam Barati 2016-11-06 14:21:51 PST
(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.
Comment 7 Saam Barati 2016-11-06 14:22:07 PST
(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.
Comment 8 Saam Barati 2016-11-06 14:23:16 PST
(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.
Comment 9 Saam Barati 2016-11-06 14:26:27 PST
Created attachment 294036 [details]
patch for landing

I'll wait until EWS is back up and running before landing the patch
Comment 10 Saam Barati 2016-11-06 14:29:19 PST
Created attachment 294037 [details]
patch for landing

fix style issues
Comment 11 WebKit Commit Bot 2016-11-09 15:26:26 PST
Comment on attachment 294037 [details]
patch for landing

Clearing flags on attachment: 294037

Committed r208496: <http://trac.webkit.org/changeset/208496>
Comment 12 WebKit Commit Bot 2016-11-09 15:26:31 PST
All reviewed patches have been landed.  Closing bug.