872735 - Parser/ParseHandler protocol cleanups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - Remove isEmptySemicolon from the ParseHandler protocol.

It's only used for a warning.

I'd be cool with using peekTokenSameLine() here because that's more precisely what this warning is trying to watch out for. Removing the warning altogether would be fine with me too. Let me know.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - Remove noteLValue from the handler protocol.

The main point of this change is to remove the noteLValue method from SyntaxParseHandler, because who knows what that is supposed to mean. Anyway it is never called, because noteLValue() was only called from Parser code that was specialized to FullParseHandler.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Part 3 - Add PNK_LABEL and js::frontend::LabeledStatement for labeled statements

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Part 4 - Remove setBinaryRHS from the ParseHandler protocol. It was only used in parsing switch statements. It is replaced by a newCase method.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Part 5 - Remove newNullary method from the ParseHandler protocol.

It is only used for array holes; replace it with a newElision method. Add PNK_ELISION to distinguish elisions from sequence expressions (which use PNK_COMMA).

(Unfortunately this is as far as the PNK-splitting goes in this stack.)

Comment 6

[Jason Orendorff [:jorendorff]]

Looking good: https://tbpl.mozilla.org/?tree=Try&rev=03e1dd56485b

Comment 7

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #1)
> I'd be cool with using peekTokenSameLine() here because that's more
> precisely what this warning is trying to watch out for. Removing the warning
> altogether would be fine with me too. Let me know.

For ifs, at least, ';' on same or separate line are equally dumb, seems to me.  Different story for loops, but we're not talking about them.

Our strict warnings are arguably more trouble than they're worth, but this one's harmless enough to keep around that I'd rather we start killing them off with a more heavyweight instance than with something harmless like this.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 760186 [details] [diff] [review]
Part 2 - Remove noteLValue from the handler protocol.

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

Might be nice to rename from noteLValue to noteIsAssigned or markAsAssigned or something like that, that's a little clearer about the meaning.

Also looks like you could assert the node in question is a name node (JOF_NAME) or something.  Separate patch if you want it, rs=me on it, probably would want a tryservering to be safe.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 760190 [details] [diff] [review]
Part 3 - Add PNK_LABEL and js::frontend::LabeledStatement for labeled statements

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

::: js/src/frontend/ParseNode.h
@@ +969,5 @@
>          return (LexicalScopeNode *) ParseNode::create(kind, PN_NAME, handler);
>      }
>  };
>  
> +class LabeledStatement : public ParseNode

OH!  YES!  http://j.mp/15RMDnj

@@ +972,5 @@
>  
> +class LabeledStatement : public ParseNode
> +{
> +  public:
> +    LabeledStatement(PropertyName *label, ParseNode *stmt, uint32_t begin)

Probably should use a Handle<PropertyName*> here, even if (I assume) while parsing moving and stuff currently is off-limits or whatever, but of course that could change eventually.

::: js/src/frontend/Parser.cpp
@@ -1072,5 @@
>      return pn;
>  }
>  
> -static void
> -ForgetUse(ParseNode *pn)

Good riddance!

@@ +4802,5 @@
>        case TOK_ERROR:
>          return null();
>  
> +      case TOK_NAME: {
> +        TokenKind next = tokenStream.peekTokenSameLine();

I might be horribly misreading the name, or misremembering the functionality, but why same-line here?  LabelledStatement is Identifier : Statement, with no [no LineTerminator here] negative lookahead production, so shouldn't this look across line breaks?  Add a test for this case, too, something like eval("label\n:do {} while (false);") if I'm understanding this correctly.

::: js/src/ion/AsmJS.cpp
@@ +167,5 @@
>      return pn->as<LoopControlStatement>().label();
>  }
>  
>  static inline PropertyName *
>  LabeledStatementLabel(ParseNode *pn)

Could you move this and the method below into methods on LabeledStatement?  There's no reason for this stuff to be in AsmJS land.

::: js/src/jsreflect.cpp
@@ +2154,5 @@
>          RootedValue label(cx), stmt(cx);
>          RootedAtom pnAtom(cx, pn->pn_atom);
>          return identifier(pnAtom, NULL, &label) &&
>                 statement(pn->pn_expr, &stmt) &&
>                 builder.labeledStatement(label, stmt, &pn->pn_pos, dst);

This stuff should use the new accessor methods in LabeledStatement.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 760191 [details] [diff] [review]
Part 4 - Remove setBinaryRHS from the ParseHandler protocol. It was only used in parsing switch statements. It is replaced by a newCase method.

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

::: js/src/frontend/FullParseHandler.h
@@ +172,5 @@
>      ParseNode *newLabeledStatement(PropertyName *label, ParseNode *stmt, uint32_t begin) {
>          return new_<LabeledStatement>(label, stmt, begin);
>      }
>  
> +    ParseNode *newCase(uint32_t begin, ParseNode *expr, ParseNode *body) {

I'd prefer newCaseOrDefault, or something, to make clear that it's handling both case and default.  Even better would be separate methods, in my book!

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 760192 [details] [diff] [review]
Part 5 - Remove newNullary method from the ParseHandler protocol.

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

Good stuff all around in all these patches!

::: js/src/frontend/FullParseHandler.h
@@ +49,5 @@
>      LazyScript * const lazyOuterFunction_;
>      size_t lazyInnerFunctionIndex;
>  
> +    const TokenPos &pos() {
> +        return tokenStream.currentToken().pos;

Might be worth a search for currentToken().pos to see if any other spots could be switched, in a separate patch.

::: js/src/jsreflect.cpp
@@ +2515,5 @@
>  
>          for (ParseNode *next = pn->pn_head; next; next = next->pn_next) {
>              JS_ASSERT(pn->pn_pos.encloses(next->pn_pos));
>  
> +            if (next->isKind(PNK_ELISION) && next->pn_count == 0) {

The second condition here should be removed.

Comment 12

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> > +      case TOK_NAME: {
> > +        TokenKind next = tokenStream.peekTokenSameLine();
> 
> I might be horribly misreading the name, or misremembering the
> functionality, but why same-line here?  LabelledStatement is Identifier :
> Statement, with no [no LineTerminator here] negative lookahead production,
> so shouldn't this look across line breaks?  Add a test for this case, too,
> something like eval("label\n:do {} while (false);") if I'm understanding
> this correctly.

Good eye. I screwed up by following the existing code there, which had to do with 'module "foo"', which *does* have the negative lookahead.

Fixed and tested. Thank you!

> ::: js/src/ion/AsmJS.cpp
> >  static inline PropertyName *
> >  LabeledStatementLabel(ParseNode *pn)
> 
> Could you move this and the method below into methods on LabeledStatement? 
> There's no reason for this stuff to be in AsmJS land.

I did add methods to LabeledStatement, but I didn't totally remove these -- the extra layer of inlines isolating AsmJS from ParseNode.h is arguably unnecessary but it's consistently used; removing this abstraction in just *one* place would be too weird. The bug to remove this is bug 854061.

> ::: js/src/jsreflect.cpp
> >          RootedAtom pnAtom(cx, pn->pn_atom);
> >          return identifier(pnAtom, NULL, &label) &&
> >                 statement(pn->pn_expr, &stmt) &&
> This stuff should use the new accessor methods in LabeledStatement.

Yeah! EmitLabeledStatement, too. It looks nice! :)

Comment 13

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> > +    ParseNode *newCase(uint32_t begin, ParseNode *expr, ParseNode *body) {
> 
> I'd prefer newCaseOrDefault, or something, to make clear that it's handling
> both case and default.  Even better would be separate methods, in my book!

Chose newCaseOrDefault. There are considerations both ways; the deciding factor was that there's only one call site and separate methods would be extra work there.

Comment 14

[Jason Orendorff [:jorendorff]]

New try run with the extra changes mentioned in comment 8.
  https://tbpl.mozilla.org/?tree=Try&rev=2061599f7778
There's a good bit of orange but it's all known. Tests pass locally.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aaed29a8aeba
https://hg.mozilla.org/mozilla-central/rev/6636ce59138e
https://hg.mozilla.org/mozilla-central/rev/4f3fdb1e4fba
https://hg.mozilla.org/mozilla-central/rev/f426ed9af960
https://hg.mozilla.org/mozilla-central/rev/c9437743a45e
https://hg.mozilla.org/mozilla-central/rev/ad385f54ee01