Last Comment Bug 710941 - Simplify parsing to the right of a dot in a property access
: Simplify parsing to the right of a dot in a property access
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla12
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on: 721322
  Show dependency treegraph
Reported: 2011-12-14 17:17 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-01-26 07:42 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (20.81 KB, patch)
2011-12-14 17:17 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-14 17:17:33 PST
Created attachment 581840 [details] [diff] [review]

Right now we reuse primary-expression parsing to parse in the |obj.<here>| and |obj..<here>| cases.  This results in us having to propagate an |afterDot| argument through |primaryExpr| to cope, complicating control flow and making it difficult to read, and harder to understand.  This also makes it harder to propagate property-is-a-PropertyName information from the tokenizer into parse nodes, and from there to the bytecode emitter.  We should parse |obj.<here>| not using the same code that parses |<here>| alone.

This adds lines of code overall, for the moment, although I argue it is much easier to understand than the previous, shorter code.  It also has the benefit of not going to substantial effort to construct a parse node for <here> if it's just a plain old name and isn't followed by ::.  (This is an adoption of simplicity from the existing non-E4X parsing path.)  I may be able to get some back by subsequently semi-unifying |obj.<here>| parsing with |obj..<here>| parsing.  But this is enough change here that an incremental patch makes sense, and in any case I think the simplicity pays for the line delta anyway.

This is atop all the patches in bug 710932.
Comment 1 User image Jason Orendorff [:jorendorff] 2011-12-30 11:34:59 PST
Oh, here's the patch I was looking for.

This looks great. I'll review today.
Comment 2 User image Jason Orendorff [:jorendorff] 2011-12-30 18:37:48 PST
Can't finish today. Tuesday.
Comment 3 User image Jason Orendorff [:jorendorff] 2012-01-03 14:42:58 PST
Comment on attachment 581840 [details] [diff] [review]

You stopped short of rewriting the TOK_DBLDOT case in memberExpr, and then eliminating the afterDot parameter of primaryExpr entirely? I don't mean to look a gift refactoring in the mouth, mind.

Comment 4 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-03 17:20:21 PST

At a certain level I remain aware that I'm doing this mostly to hasten the property storage split, so I'm wary of doing more work than necessary that exceeds the needs of that particular problem.
Comment 5 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-03 20:32:37 PST
Comment 6 User image Marco Bonardo [::mak] 2012-01-04 04:52:31 PST

Note You need to log in before you can comment on or make changes to this bug.