Closed
Bug 461269
Opened 13 years ago
Closed 12 years ago
Remove TOK_RP nodes from the parse tree
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jorendorff, Assigned: jorendorff)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 4 obsolete files)
33.39 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Jason, could you refresh and attach that patch? Thanks, /be
Assignee | ||
Comment 2•12 years ago
|
||
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•12 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 | ||
Comment 4•12 years ago
|
||
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
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
Attachment #392328 -
Attachment is obsolete: true
Attachment #393296 -
Flags: review?(brendan)
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to comment #5) > No waste with current fixed-size allocator, right? Right.
Comment 8•12 years ago
|
||
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•12 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•12 years ago
|
||
Interdiff should work.
Attachment #393296 -
Attachment is obsolete: true
Attachment #393342 -
Flags: review?(brendan)
Assignee | ||
Comment 11•12 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)
Comment 12•12 years ago
|
||
Take another bit from pn_arity if you need to. /be
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #393342 -
Attachment is obsolete: true
Attachment #393525 -
Flags: review?(brendan)
Comment 14•12 years ago
|
||
Comment on attachment 393525 [details] [diff] [review] v5 Great, thanks! /be
Attachment #393525 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 20•12 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/ffbcd7c55130
Whiteboard: fixed-in-tracemonkey
Comment 21•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ffbcd7c55130
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•