Closed Bug 630232 Opened 9 years ago Closed 8 years ago

Reflect.parse("[,,,]") should make an ArrayExpression with .elements.length 3

Categories

(Core :: JavaScript Engine, defect, P5)

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(1 file)

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
Attached patch v1Splinter Review
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 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+
https://hg.mozilla.org/mozilla-central/rev/21f05e099a36
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.