Closed
Bug 630232
Opened 14 years ago
Closed 13 years ago
Reflect.parse("[,,,]") should make an ArrayExpression with .elements.length 3
Categories
(Core :: JavaScript Engine, defect, P5)
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: jorendorff, Assigned: jorendorff)
Details
Attachments
(1 file)
4.92 KB,
patch
|
dherman
:
review+
|
Details | Diff | Splinter Review |
Reflect.parse("[]") and Reflect.parse("[,,,]") return identical ASTs, even though those are two different programs.
js> eval("[]").length
0
js> eval("[,,,]").length
3
js> assertEq(Reflect.parse("[,,,]").body[0].expression.elements.length, 3)
typein:11: Error: Assertion failed: got 0, expected 3
Assignee | ||
Comment 1•14 years ago
|
||
Taking, since I have a patch lying around.
This bug would have been detected by the tests, but the testing mechanism had a minor bug. Fixing that bug caused the tests to detect bug 638577 (i.e. fail because of it)! So I had to comment out a few lines of tests.
Assignee: dherman → jorendorff
Attachment #516703 -
Flags: review?(dherman)
Comment 2•13 years ago
|
||
Comment on attachment 516703 [details] [diff] [review]
v1
>- JSObject *array = NewDenseEmptyArray(cx);
>+ const size_t len = elts.length();
>+ JSObject *array = NewDenseAllocatedArray(cx, uint(len));
Neat -- didn't know about NewDenseAllocatedArray.
> if (!array)
> return false;
>
>- const size_t len = elts.length();
> for (size_t i = 0; i < len; i++) {
> Value val = elts[i];
>
> JS_ASSERT_IF(val.isMagic(), val.whyMagic() == JS_SERIALIZE_NO_NODE);
>
> /* Represent "no node" as an array hole by not adding the value. */
> if (val.isMagic(JS_SERIALIZE_NO_NODE))
> continue;
Aaaaargh, fencepost bug! I haz a sad.
>-assertExpr("[,,,1,2,3,]", arrExpr([,,,lit(1),lit(2),lit(3)]));
>-assertExpr("[,,,1,2,3,,]", arrExpr([,,,lit(1),lit(2),lit(3),]));
>-assertExpr("[,,,1,2,3,,,]", arrExpr([,,,lit(1),lit(2),lit(3),,]));
>-assertExpr("[,,,,,]", arrExpr([,,,,]));
>+assertExpr("[,,,1,2,3,]", arrExpr([,,,lit(1),lit(2),lit(3),]));
>+assertExpr("[,,,1,2,3,,]", arrExpr([,,,lit(1),lit(2),lit(3),,]));
>+assertExpr("[,,,1,2,3,,,]", arrExpr([,,,lit(1),lit(2),lit(3),,,]));
>+assertExpr("[,,,,,]", arrExpr([,,,,,]));
Bogus test case is bogus. Mea culpa.
Looks good! r=me
Attachment #516703 -
Flags: review?(dherman) → review+
Assignee | ||
Comment 3•13 years ago
|
||
Comment 4•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in
before you can comment on or make changes to this bug.
Description
•