Closed Bug 872735 Opened 7 years ago Closed 7 years ago

Parser/ParseHandler protocol cleanups

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(5 files)

No description provided.
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.
Assignee: general → jorendorff
Attachment #760185 - Flags: review?(jwalden+bmo)
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.
Attachment #760186 - Flags: review?(jwalden+bmo)
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.)
Attachment #760192 - Flags: review?(jwalden+bmo)
Attachment #760185 - Flags: review?(jwalden+bmo) → review+
(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 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.
Attachment #760186 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #760190 - Flags: review?(jwalden+bmo) → review+
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!
Attachment #760191 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #760192 - Flags: review?(jwalden+bmo) → review+
(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! :)
(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.
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.
Depends on: 883523
Depends on: 883643
You need to log in before you can comment on or make changes to this bug.