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
Status: RESOLVED FIXED
:
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]
Mentors:
Depends on: 721322
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-14 17:17:33 PST
Created attachment 581840 [details] [diff] [review]
Patch

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 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 Jason Orendorff [:jorendorff] 2011-12-30 18:37:48 PST
Can't finish today. Tuesday.
Comment 3 Jason Orendorff [:jorendorff] 2012-01-03 14:42:58 PST
Comment on attachment 581840 [details] [diff] [review]
Patch

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.

r=me
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-03 17:20:21 PST
Heh.

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 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-03 20:32:37 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/377a1042074e
Comment 6 Marco Bonardo [::mak] 2012-01-04 04:52:31 PST
https://hg.mozilla.org/mozilla-central/rev/377a1042074e

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