Last Comment Bug 877908 - Confusing error message from "use asm" with "use strict"
: Confusing error message from "use asm" with "use strict"
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- minor (vote)
: mozilla24
Assigned To: Benjamin Bouvier [:bbouvier]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-30 16:52 PDT by Jesse Ruderman
Modified: 2013-06-07 12:32 PDT (History)
1 user (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix + test (2.61 KB, patch)
2013-05-31 14:33 PDT, Benjamin Bouvier [:bbouvier]
luke: review+
Details | Diff | Splinter Review
same fix addressing comments (2.62 KB, patch)
2013-06-06 13:50 PDT, Benjamin Bouvier [:bbouvier]
bbouvier: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2013-05-30 16:52:38 PDT
js> (function() { "use asm"; "use strict"; function g(){} return g; })

typein:14:0 warning: asm.js type error: asm.js module must end with a return export statement:
typein:14:0 warning: (function() { "use asm"; "use strict"; function g(){} return
typein:14:0 warning: .^

Expected one of the following:
* "use asm" precludes other directives
* unsupported kind of statement in asm.js module
Comment 1 Benjamin Bouvier [:bbouvier] 2013-05-31 14:33:22 PDT
Created attachment 756786 [details] [diff] [review]
proposed fix + test

The asmjs spec is not precise whether the use of "use asm" should preclude other directives, so this patch stays consistent with the rest of the function.
Comment 2 Luke Wagner [:luke] 2013-06-05 17:59:24 PDT
Comment on attachment 756786 [details] [diff] [review]
proposed fix + test

Review of attachment 756786 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/ion/AsmJS.cpp
@@ +2574,5 @@
> +    *stmtIter = NextNonEmptyStatement(firstStatement);
> +    if (*stmtIter
> +        && IsExpressionStatement(*stmtIter)
> +        && ExpressionStatementExpr(*stmtIter)->isKind(PNK_STRING))
> +        return m.fail(*stmtIter, "\"use asm\" precludes other directives");

SM style is, when the condition is multi-line, { brace } the then branch, even if it is a single line (with the opening { on a new line).  So,

if (....
    ...)
{
    return ...
}
Comment 3 Benjamin Bouvier [:bbouvier] 2013-06-06 13:50:31 PDT
Created attachment 759393 [details] [diff] [review]
same fix addressing comments

Carrying r+ with fixed nit.
Comment 4 Ryan VanderMeulen [:RyanVM] 2013-06-07 05:25:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a027c965a33
Comment 5 Ryan VanderMeulen [:RyanVM] 2013-06-07 12:32:04 PDT
https://hg.mozilla.org/mozilla-central/rev/0a027c965a33

Note You need to log in before you can comment on or make changes to this bug.