Last Comment Bug 713755 - Use constructors to create a.<here> and a[<here>] parse nodes
: Use constructors to create a.<here> and a[<here>] parse nodes
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)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-27 14:28 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-01-04 04:53 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Rename a bunch of variables in memberExpr (8.77 KB, patch)
2011-12-27 14:30 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review
Use constructors to parse a.<here> (13.17 KB, patch)
2011-12-27 14:32 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review
Use constructors to parse a[...] (6.46 KB, patch)
2011-12-27 14:33 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-27 14:28:40 PST

    
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-27 14:30:23 PST
Created attachment 584486 [details] [diff] [review]
Rename a bunch of variables in memberExpr

There's one variable in here that gets renamed in a later patch, once I understood more how the different twisty passages all alike were used.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-27 14:32:10 PST
Created attachment 584487 [details] [diff] [review]
Use constructors to parse a.<here>
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-27 14:33:18 PST
Created attachment 584488 [details] [diff] [review]
Use constructors to parse a[...]
Comment 4 Jason Orendorff [:jorendorff] 2011-12-30 09:26:56 PST
Comment on attachment 584486 [details] [diff] [review]
Rename a bunch of variables in memberExpr

Is this based on a patches that aren't pushed yet? It looks like the control flow in memberExpr is different in your patch than on tip.

Still, I can tell from the indentation that this won't break anything.
Comment 5 Jason Orendorff [:jorendorff] 2011-12-30 10:06:15 PST
Comment on attachment 584487 [details] [diff] [review]
Use constructors to parse a.<here>

Looks good.

I would be OK with DoubleColonProperty and FilterExpression being named XMLDoubleColonProperty and XMLFilterExpression just so people can tell what they are from far off.

I don't think nextMember is a great name for that. Really it means "the enclosing expression of which lhs is the left-hand side". It could be called "parent" or "outer"; I guess those are not great names either. Oh well.
Comment 6 Jason Orendorff [:jorendorff] 2011-12-30 11:25:44 PST
Comment on attachment 584488 [details] [diff] [review]
Use constructors to parse a[...]

As far as I can tell, this affects the bytecode emitted in two cases:

  - Non-index numbers
    obj[3.14] currently emits 'getgname obj; double 3.14; getelem'.
    With the patch, it will emit 'getgname obj; string "3.14"; getelem'.
    Same goes for obj[0x100000000] and possibly obj[-1] (not sure).
    This change is fine.

  - Index strings
    obj['0'] currently emits 'getgname obj; zero; getelem'.
    With the patch, it will emit 'getgname obj; string "0"; getelem'.
    This seems worse to me.

In tests/js1_5/extensions/regress-333541.js:
>     "    x = (1)['\"a\"'];\n" +
>     "    x = (1)[\'\\'a\\''];\n" +
>-    "    x = (1)['1'];\n" +
>+    "    x = (1)[\"1\"];\n" +
>     "    x = (1)['#'];\n" +
>     "}";

I don't understand the change here.

r=me with those two things explained or fixed.

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