Closed Bug 883226 Opened 7 years ago Closed 7 years ago

Further Parser/ParseHandler protocol cleanups: primary and unary expressions

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(8 files, 1 obsolete file)

Should be about 8 patches in this stack.
There was some copy-and-paste in the last-minute change for bug 678037. This tidies that up among other things.
Assignee: general → jorendorff
Attachment #762759 - Flags: review?(jwalden+bmo)
When templates are involved, things like default arguments and method overloading go from niceties to annoyances.

It's less clear what's going on. And invariants about when obj.method(a, b) is equivalent to obj.method(a) are spread across all implementing classes rather than centrally enforced.

This gives newNumber a single signature and adds a single helper function in Parser for all other cases.

Also, irrelevantly, saves a bunch of code in Parser::primaryExpr() by returning from each case in the switch, rather than breaking.
Attachment #762764 - Flags: review?(jwalden+bmo)
Removes setUnaryKid!
Attachment #762770 - Flags: review?(jwalden+bmo)
The work done in foldPropertyByValue still exists, of course, but with this patch, it's entirely internal to FullParseHandler; the parser itself does not know about this optimization at all. :)
Attachment #762786 - Flags: review?(jwalden+bmo)
Also fixes bug 871046.
Attachment #762789 - Flags: review?(jwalden+bmo)
Attached patch Part 6 - Strings, v1 (obsolete) — Splinter Review
Attachment #762792 - Flags: review?(jwalden+bmo)
(quick v2 - found a few more things easily cleaned up after this change)
Attachment #762792 - Attachment is obsolete: true
Attachment #762792 - Flags: review?(jwalden+bmo)
Attachment #762796 - Flags: review?(jwalden+bmo)
newIdentifier is used for the X in ({X: 1}). A better name for that method would be welcome...
Attachment #762802 - Flags: review?(jwalden+bmo)
The mission here is decoupling. Currently the ParseHandler protocol mentions ParseContext<ParseHandler> in two places. With this patch, only one place, and that's only for asm.js, so it should go away soon.
Attachment #762806 - Flags: review?(jwalden+bmo)
And that's the stack. More to come in a separate bug...
Comment on attachment 762759 [details] [diff] [review]
Part 1 - RegExp, v1

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

::: js/src/frontend/ParseNode.h
@@ -371,5 @@
>   *                          node in PNK_OBJECT's list, has PNX_DESTRUCT flag
>   * PNK_NAME,    name        pn_atom: name, string, or object atom
> - * PNK_STRING,              pn_op: JSOP_NAME, JSOP_STRING, or JSOP_OBJECT, or
> - *                                 JSOP_REGEXP
> - * PNK_REGEXP               If JSOP_NAME, pn_op may be JSOP_*ARG or JSOP_*VAR

wat

::: js/src/frontend/SyntaxParseHandler.h
@@ +70,5 @@
>      Node newThisLiteral(const TokenPos &pos) { return NodeGeneric; }
>      Node newNullLiteral(const TokenPos &pos) { return NodeGeneric; }
> +
> +    template <class Boxer>
> +    Node newRegExp(JSObject *reobj, const TokenPos &pos, Boxer &boxer) { return NodeGeneric; }

I have Simon & Garfunkel running through my head now.
Attachment #762759 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 762764 [details] [diff] [review]
Part 2 - newNumber, v1

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

It seems slightly weird to have both newNumber and handler.newNumber as things being used.  Not sure what could be done about it, or even if something necessarily should be done about it.
Attachment #762764 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 762770 [details] [diff] [review]
Part 3 - newUnary, v1

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

::: js/src/frontend/FullParseHandler.h
@@ +148,5 @@
>          return new_<NullaryNode>(PNK_ELISION, pos());
>      }
>  
> +    ParseNode *newUnary(ParseNodeKind kind, JSOp op, uint32_t begin, ParseNode *kid) {
> +        TokenPos pos = {begin, kid ? kid->pn_pos.end : begin + 1};

Probably should be ()-constructed for consistency with stuff in that other patch I reviewed.

::: js/src/frontend/Parser.cpp
@@ +1050,5 @@
>          JS_ASSERT(JS_HAS_EXPR_CLOSURES);
>  
> +        tokenStream.getToken();
> +        uint32_t begin = pos().begin;
> +        tokenStream.ungetToken();

AUGHAUAHGGHAUGH this is awful.  :-(

@@ +5428,5 @@
>  
>      JS_CHECK_RECURSION(context, return null());
>  
> +    TokenKind tt = tokenStream.getToken(TSF_OPERAND);
> +    uint32_t begin = pos().begin;

Yeah, this whole passing in begin thing here is kinda fruity.  I dunno, maybe this really is the only way to do it when you're syntax-parsing, tho, because Node doesn't have position info at hand?

@@ +6595,5 @@
>                  } else if (tt == TOK_TRIPLEDOT) {
>                      spread = true;
>                      handler.setListFlag(pn, PNX_SPECIALARRAYINIT | PNX_NONCONST);
>  
>                      tokenStream.getToken();

Change this to consumeKnownToken(tt).
Attachment #762770 - Flags: review?(jwalden+bmo) → review+
Attachment #762786 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> It seems slightly weird to have both newNumber and handler.newNumber as
> things being used.  Not sure what could be done about it, or even if
> something necessarily should be done about it.

I could rename Parser::newNumber(const Token &) to number() or numberForCurrentToken().

(The reason it's not a handler method is because then it'd have to be defined on two classes rather than one, and the invariant that newNumber(token) === newNumber(token.number(), token.decimalPoint(), token.pos) would be distributed. Also to keep trying to make the protocol more like what you'd expect from looking at ECMA-262 Annex A sections 3-5, and less like a bowl of spaghetti. Admittedly all these concerns are pretty abstract.)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> > +        TokenPos pos = {begin, kid ? kid->pn_pos.end : begin + 1};
> 
> Probably should be ()-constructed for consistency with stuff in that other
> patch I reviewed.

That patch goes on top of these. And worry not, it definitely switches to (), because C++ won't let you use ={} once there's a nontrivial constructor. Or something.

> ::: js/src/frontend/Parser.cpp
> @@ +1050,5 @@
> >          JS_ASSERT(JS_HAS_EXPR_CLOSURES);
> >  
> > +        tokenStream.getToken();
> > +        uint32_t begin = pos().begin;
> > +        tokenStream.ungetToken();
> 
> AUGHAUAHGGHAUGH this is awful.  :-(

Maybe the right answer is to add a newExpressionClosure() signature to the ParseHandler protocol. Then FullParseHandler would be responsible for creating the implicit PNK_RETURN node and setting its position.

In the meantime, I could change it to use handler.setBeginPosition() later instead of this grossness. But I'm kind of trying to get rid of setBeginPosition and friends.

> > +    TokenKind tt = tokenStream.getToken(TSF_OPERAND);
> > +    uint32_t begin = pos().begin;
> 
> Yeah, this whole passing in begin thing here is kinda fruity.  I dunno,
> maybe this really is the only way to do it when you're syntax-parsing, tho,
> because Node doesn't have position info at hand?

Nah, syntax-parsing doesn't care. The reason I'm doing this is to get it right when parsing for real. Currently we get it wrong:

  js> Reflect.parse("x").body[0].expression.loc.start.column
  0
  js> Reflect.parse("-x").body[0].expression.loc.start.column
  1

The stack fixes a lot of location information that has been slightly off.

> >                      tokenStream.getToken();
> 
> Change this to consumeKnownToken(tt).

Yup.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> ::: js/src/frontend/Parser.cpp
> @@ +1050,5 @@
> >          JS_ASSERT(JS_HAS_EXPR_CLOSURES);
> >  
> > +        tokenStream.getToken();
> > +        uint32_t begin = pos().begin;
> > +        tokenStream.ungetToken();
> 
> AUGHAUAHGGHAUGH this is awful.  :-(

Somehow I lost this when commenting, but is this actually safe when EOF is hit here?  I'd be a bit surprised if it were!  Add a test either way.  (Do we have an assert if you getToken when you're at EOF right now, hopefully?)
Comment on attachment 762789 [details] [diff] [review]
Part 5 - Delete expressions.

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

::: js/src/frontend/Parser.cpp
@@ +5410,1 @@
>              return null();

Folding constants before doing this syntax-based error reporting is really dodgy.  I guess it's preexisting, tho, so I should hold my nose some and hope we return to fix this at some point.  :-\

Or could we fix this, cleanly, by moving the isName check up above the foldConstants?  Perhaps a separate patch/bug, but it'd be nice to try that, and make the constant-folding as wholly alternate to the isName cases.

@@ +6065,2 @@
>  bool
> +Parser<ParseHandler>::arrayInitializerComprehensionTail(Node pn)

Why this change?  Looked right to me as it was before.

::: js/src/tests/js1_8_5/regress/regress-609617.js
@@ -71,5 @@
>      assertEq(e.message, "invalid assignment left-hand side");
>  }
>  assertEq(fooArg, 'x');
>  
> -/* We extend ES5 by making delete of a call expression a strict mode error. */

wat
Attachment #762789 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 762796 [details] [diff] [review]
Part 6 - Strings, v2

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

::: js/src/frontend/Parser.cpp
@@ +6630,5 @@
>                              atom = ToAtom<CanGC>(context, HandleValue::fromMarkedLocation(&tmp));
>                              if (!atom)
>                                  return null();
>                          } else {
> +                            pn3 = stringLiteral();

I might prefer if this weren't overloading string literals here but were a different kind entirely, but that probably requires another patch to introduce that distinction, or something.  Object literals need a bunch more kind customization, if I remember.
Attachment #762796 - Flags: review?(jwalden+bmo) → review+
Attachment #762802 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 762806 [details] [diff] [review]
Part 8 - NameNode constructor, v1

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

::: js/src/frontend/ParseNode.h
@@ +954,5 @@
>  };
>  
>  struct NameNode : public ParseNode
>  {
> +    NameNode(ParseNodeKind kind, JSOp op, JSAtom *atom, bool inBlock, uint32_t blockid,

Please make |inBlock| into an enum of some sort.  Followup patch, maybe, probably.

@@ -958,4 @@
>      static NameNode *create(ParseNodeKind kind, JSAtom *atom,
> -                            FullParseHandler *handler, ParseContext<FullParseHandler> *pc);
> -
> -    inline void initCommon(ParseContext<FullParseHandler> *pc);

Woo, no more initCommon garbage!
Attachment #762806 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18)
> Comment on attachment 762789 [details] [diff] [review]
> Part 5 - Delete expressions.
> 
> Review of attachment 762789 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/Parser.cpp
> @@ +5410,1 @@
> >              return null();
> 
> Folding constants before doing this syntax-based error reporting is really
> dodgy.  I guess it's preexisting, tho, so I should hold my nose some and
> hope we return to fix this at some point.  :-\

Yup. I'll fix it in bug 844805.

> >  bool
> > +Parser<ParseHandler>::arrayInitializerComprehensionTail(Node pn)
> 
> Why this change?  Looked right to me as it was before.

You're right. Changed it back.

This was part of a stack that eventually added a DiagnosticParseHandler (a third ParseHandler class), so that change got mixed in here by accident.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> > +        tokenStream.getToken();
> > +        uint32_t begin = pos().begin;
> > +        tokenStream.ungetToken();
> 
> Somehow I lost this when commenting, but is this actually safe when EOF is
> hit here?

Or a tokenization error - *yes*, it works. getToken() allocates a new Token in all circumstances. pos().begin is the offset of the EOF or error, and ungetToken() puts the TOK_EOF or TOK_ERROR token back.

I'll add a test.

> (Do we have an assert if you getToken when you're at EOF right now, hopefully?)

You mean, getToken() after TOK_EOF or TOK_ERROR? No, we don't have an assertion against that, and it might help to add one (see bug 438587).
You need to log in before you can comment on or make changes to this bug.