Closed Bug 461269 Opened 12 years ago Closed 11 years ago

Remove TOK_RP nodes from the parse tree

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 4 obsolete files)

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
Attached patch work in progress (obsolete) — Splinter Review
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.)
(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.
Blocks: 475849
Blocks: 460158
Blocks: 373672
Attached patch WIP 2 (obsolete) — Splinter Review
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
Attached patch v3 (obsolete) — Splinter Review
Attachment #392328 - Attachment is obsolete: true
Attachment #393296 - Flags: review?(brendan)
(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+
(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. :-\
Attached patch v4 (obsolete) — Splinter Review
Interdiff should work.
Attachment #393296 - Attachment is obsolete: true
Attachment #393342 - Flags: review?(brendan)
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
Attached patch v5Splinter Review
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+
Duplicate of this bug: 373672
Duplicate of this bug: 352304
Duplicate of this bug: 460158
Duplicate of this bug: 475843
Duplicate of this bug: 475849
http://hg.mozilla.org/tracemonkey/rev/ffbcd7c55130
Whiteboard: fixed-in-tracemonkey
Depends on: 509636
http://hg.mozilla.org/mozilla-central/rev/ffbcd7c55130
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.