Remove TOK_RP nodes from the parse tree

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

10 years ago
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.
Jason, could you refresh and attach that patch? Thanks,

/be
(Assignee)

Comment 2

10 years ago
Created attachment 359413 [details] [diff] [review]
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.)
(Assignee)

Comment 3

10 years ago
(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.
(Assignee)

Updated

10 years ago
Blocks: 475849

Updated

10 years ago
Blocks: 460158
Blocks: 373672
(Assignee)

Comment 4

9 years ago
Created attachment 392328 [details] [diff] [review]
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.
Assignee: general → jorendorff
Attachment #359413 - Attachment is obsolete: true
(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
(Assignee)

Comment 6

9 years ago
Created attachment 393296 [details] [diff] [review]
v3
Attachment #392328 - Attachment is obsolete: true
Attachment #393296 - Flags: review?(brendan)
(Assignee)

Comment 7

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

Right.
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
Attachment #393296 - Flags: review?(brendan) → review+
(Assignee)

Comment 9

9 years ago
(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. :-\
(Assignee)

Comment 10

9 years ago
Created attachment 393342 [details] [diff] [review]
v4

Interdiff should work.
Attachment #393296 - Attachment is obsolete: true
Attachment #393342 - Flags: review?(brendan)
(Assignee)

Comment 11

9 years ago
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.
Attachment #393342 - Flags: review?(brendan)
Take another bit from pn_arity if you need to.

/be
(Assignee)

Comment 13

9 years ago
Created attachment 393525 [details] [diff] [review]
v5
Attachment #393342 - Attachment is obsolete: true
Attachment #393525 - Flags: review?(brendan)
Comment on attachment 393525 [details] [diff] [review]
v5

Great, thanks!

/be
Attachment #393525 - Flags: review?(brendan) → review+
(Assignee)

Updated

9 years ago
Duplicate of this bug: 373672
(Assignee)

Updated

9 years ago
Duplicate of this bug: 352304
(Assignee)

Updated

9 years ago
Duplicate of this bug: 460158
(Assignee)

Updated

9 years ago
Duplicate of this bug: 475843
(Assignee)

Updated

9 years ago
Duplicate of this bug: 475849
(Assignee)

Comment 20

9 years ago
http://hg.mozilla.org/tracemonkey/rev/ffbcd7c55130
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

9 years ago
Depends on: 509636
Depends on: 510709

Comment 21

9 years ago
http://hg.mozilla.org/mozilla-central/rev/ffbcd7c55130
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 22

9 years ago
js/tests/js1_8_1/extensions/strict-warning.js
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.