Closed
Bug 1206980
Opened 8 years ago
Closed 7 years ago
Separate MatchOrInsertSemicolon into 2 functions for after expression and non-expression.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
Details
Attachments
(1 file)
Currently MatchOrInsertSemicolon receives modifier argument, but it would be better to have 2 functions for Operand and None case, with explicit name for each situation. ASI happens in following situations (where MatchOrInsertSemicolon is called): modifier is None, ends with expression * function declaration with expression closure * var declaration * let declaration * export declaration with var * export declaration with default, without function or class * export declaration with default, function expression (handled in functionStmt) * expression statement * return statement with expression * throw statement modifier is Operand, ends with non-expression * import declaration, which ends with module name string * export declaration with from (including {...} and *), which ends with module name string (bug 1206750) * export declaration without from, peeked token for |from| is Operand * continue statement, which ends with optional label identifier, peeked token for label is Operand * break statement, which ends with optional label identifier, peeked token for label is Operand * return statement without expression, peeked token for expression is Operand * debugger statement
Assignee | ||
Comment 1•7 years ago
|
||
Added MatchOrInsertSemicolonAfterExpression and MatchOrInsertSemicolonAfterNonExpression, each for TokenStream::None and TokenStream::Operand. Both of them call MatchOrInsertSemicolon internally. Might be better to rename MatchOrInsertSemicolon to something else now? (or any other way to hide it?)
Assignee: nobody → arai.unmht
Attachment #8665981 -
Flags: review?(jwalden+bmo)
Comment 2•7 years ago
|
||
Comment on attachment 8665981 [details] [diff] [review] Separate MatchOrInsertSemicolon into 2 functions for after expression and non-expression. Review of attachment 8665981 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +1469,2 @@ > static bool > +MatchOrInsertSemicolon(TokenStream& ts, TokenStream::Modifier modifier) Tack a "Helper" onto the name so people are less likely to call it? That'll also break wrongly-rebased in-progress patches. @@ +4995,5 @@ > if (!binding) > return null(); > kid = assignExpr(InAllowed, YieldIsKeyword); > if (kid) { > + if (!MatchOrInsertSemicolonAfterExpression(tokenStream)) This looks (preexistingly) wrong. Shouldn't it be kid = assignExpr(...); if (!kid || !Match...) return null(); Probably fix in a followup micro-rev (r=me), for sanity.
Attachment #8665981 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 3•7 years ago
|
||
Thank you for reviewing :D (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2) > @@ +4995,5 @@ > > if (!binding) > > return null(); > > kid = assignExpr(InAllowed, YieldIsKeyword); > > if (kid) { > > + if (!MatchOrInsertSemicolonAfterExpression(tokenStream)) > > This looks (preexistingly) wrong. Shouldn't it be > > kid = assignExpr(...); > if (!kid || !Match...) > return null(); > > Probably fix in a followup micro-rev (r=me), for sanity. I feel it's still a little confusing, as |if (!kid) return null();| also exists after the |switch|. Maybe we should move it into each |case| in |switch| like following? switch (tt) { case TOK_FUNCTION: kid = functionStmt(YieldIsKeyword, AllowDefaultName); if (!kid) return null(); break; case TOK_CLASS: kid = classDefinition(YieldIsKeyword, ClassStatement, AllowDefaultName); if (!kid) return null(); break; default: tokenStream.ungetToken(); RootedPropertyName name(context, context->names().starDefaultStar); binding = makeInitializedLexicalBinding(name, true, pos()); if (!binding) return null(); kid = assignExpr(InAllowed, YieldIsKeyword); if (!kid) return null(); if (!MatchOrInsertSemicolonAfterExpression(tokenStream)) return null(); break; }
Assignee | ||
Comment 4•7 years ago
|
||
will file a separated bug for it, for clarity.
Comment 6•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/10194aec7255
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•