Constant folding should happen before deciding whether to turn obj["A"] into obj.A

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: evilpie)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
mozilla17
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
jsfunfuzz's round-trip checker caught this:

js> (function() { return t["a" + "b"]; })
(function () {return t["ab"];})
js> (function () {return t["ab"];})
(function () {return t.ab;})
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Comment 1

5 years ago
Another instance:

js> (function() { return t[true ? 'a' : 'b']; })
(function () {return t["a"];})
js> (function () {return t["a"];})
(function () {return t.a;})
(Assignee)

Comment 2

5 years ago
Created attachment 642207 [details] [diff] [review]
v1

Just do the folding manually before we decide whether to use obj.prop or obj[prop].

Really short patch, I hope this is okay for you Jeff.
Assignee: general → evilpies
Status: NEW → ASSIGNED
Attachment #642207 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

5 years ago
Created attachment 642208 [details] [diff] [review]
v1 with tests

Sorry forgot to hg add the test.
Attachment #642207 - Attachment is obsolete: true
Attachment #642207 - Flags: review?(jwalden+bmo)
Attachment #642208 - Flags: review?(jwalden+bmo)
Comment on attachment 642208 [details] [diff] [review]
v1 with tests

Review of attachment 642208 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +5833,5 @@
>                  }
>              } else if (propExpr->isKind(PNK_NUMBER)) {
> +                Value number = NumberValue(propExpr->pn_dval);
> +                uint32_t dummy;
> +                if (!IsDefinitelyIndex(number, &dummy)) {

Please use (propExpr->pn_dval == ToUint32(propExpr->pn_dval)) to check for index-ness -- much better than relying on a method that sometimes gives the wrong answer and must itself be double-checked.

::: js/src/jit-test/tests/basic/testFoldPropertyAccess.js
@@ +17,5 @@
> +    }
> +]
> +
> +for (var i = 0; i < cases.length; i++) {
> +    dis(cases[i]);

Remove this.
Attachment #642208 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 5

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/28711b9f49cd
https://hg.mozilla.org/mozilla-central/rev/28711b9f49cd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.