461269 - Remove TOK_RP nodes from the parse tree

Status: RESOLVED FIXED

Description

[Jason Orendorff [:jorendorff]]

Now that we're no longer emitting JSOP_GROUP, these can go.  I have a patch that's about halfway there.  There are only a few tricks.

Comment 1

[Brendan Eich [:brendan]]

Jason, could you refresh and attach that patch? Thanks,

/be

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: work in progress

This is probably stale.

(I don't see anything tricky in this patch but I seem to remember there being a trick of some sort, perhaps in parts of the file yet to be edited.)

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to comment #2)
> (I don't see anything tricky in this patch but I seem to remember there being a
> trick of some sort, perhaps in parts of the file yet to be edited.)

...like, maybe having to do with code implicitly assuming that a TOK_RP node is there in certain places (and discarding it).  All very hazy.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: WIP 2

Refreshed. But there's a bug: the parser bogusly complains that yield-expressions aren't parenthesized (JSMSG_BAD_GENERATOR_SYNTAX with js_yield_str).

I could add a bit to unary parse nodes for this. There's already a JSBool `hidden` that wastes a few bits.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> Created an attachment (id=392328) [details]
> WIP 2
> 
> Refreshed. But there's a bug: the parser bogusly complains that
> yield-expressions aren't parenthesized (JSMSG_BAD_GENERATOR_SYNTAX with
> js_yield_str).

Add a flag. Bitfield would be the modern thing.

> I could add a bit to unary parse nodes for this. There's already a JSBool
> `hidden` that wastes a few bits.

No waste with current fixed-size allocator, right?

/be

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: v3

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to comment #5)
> No waste with current fixed-size allocator, right?

Right.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 393296 [details] [diff] [review]
v3

>            if (pn2->pn_arity == PN_NULLARY && pn2->pn_type == TOK_COMMA) {

Nit: canonical order tests type then arity to refine.

No js_EmitTree switch case TOK_RP: /* XXX */ leftovers -- JS_ASSERT(PN_TYPE(pn) != TOK_RP) in default: instead.

s/pn3/pn2/g or better, s/pn3/rhs/ in AssignExpr.

Nice work in MemberExpr!

Great to get rid of StartsWith (as well as all the TOK_RP-skipping loops).

r=me with these picked.

/be

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to comment #8)
> Nice work in MemberExpr!

Thanks. That was the trick I had forgotten about in comment 2.

js/tests found a bug:

  ((a, b) for (i in []))   // need to accept this
  (a, b for (i in []))     // and reject this

In v3 I'm rejecting both. The parser detects the latter case like this: Expr parses `a, b`; then ParenExpr detects `for`, sees that the LHS is a TOK_COMMA expression, and errors out. This of course depends on the LHS being a TOK_RP node if it's parenthesized.

Mulling this over. I'm afraid there will be enough interdiff to warrant a new review. :-\

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: v4

Interdiff should work.

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 393342 [details] [diff] [review]
v4

As soon as I posted this mrbkap pointed out a bug:

options('strict');
options('werror');
if ((a=0)) ;

typein:3: SyntaxError: test for equality (==) mistyped as assignment (=)?:
typein:3: if ((a=0)) ;
typein:3: .........^

I guess all parse nodes should have the pn_parens bit? Binary nodes, anyway.

Comment 12

[Brendan Eich [:brendan]]

Take another bit from pn_arity if you need to.

/be

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: v5

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 393525 [details] [diff] [review]
v5

Great, thanks!

/be

Comment 20

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/ffbcd7c55130

Comment 21

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ffbcd7c55130

Comment 22

[Bob Clary [:bc:]]

js/tests/js1_8_1/extensions/strict-warning.js