Closed Bug 1206980 Opened 6 years ago Closed 6 years ago

Separate MatchOrInsertSemicolon into 2 functions for after expression and non-expression.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

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
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 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+
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;
        }
will file a separated bug for it, for clarity.
See Also: → 1210295
https://hg.mozilla.org/mozilla-central/rev/10194aec7255
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.