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

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Comment hidden (empty)
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.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #584486 - Flags: review?(jorendorff)
Created attachment 584487 [details] [diff] [review]
Use constructors to parse a.<here>
Attachment #584487 - Flags: review?(jorendorff)
Created attachment 584488 [details] [diff] [review]
Use constructors to parse a[...]
Attachment #584488 - Flags: review?(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.
Attachment #584486 - Flags: review?(jorendorff) → review+
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.
Attachment #584487 - Flags: review?(jorendorff) → review+
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.
Attachment #584488 - Flags: review?(jorendorff) → review+
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
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/ff44100cc3c3
https://hg.mozilla.org/mozilla-central/rev/214d94667876
https://hg.mozilla.org/mozilla-central/rev/a45349a7d74a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.