713755 - Use constructors to create a.<here> and a[<here>] parse nodes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: 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]]

Attachment: Use constructors to parse a.<here>

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Use constructors to parse a[...]

Comment 4

[Jason Orendorff [:jorendorff]]

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]]

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]]

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.

Comment 7

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff44100cc3c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/214d94667876
https://hg.mozilla.org/integration/mozilla-inbound/rev/a45349a7d74a

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/ff44100cc3c3
https://hg.mozilla.org/mozilla-central/rev/214d94667876
https://hg.mozilla.org/mozilla-central/rev/a45349a7d74a