Closed
Bug 1121391
Opened 9 years ago
Closed 9 years ago
Update Array.from to match the spec
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: 446240525, Assigned: 446240525)
References
Details
Attachments
(1 file, 2 obsolete files)
6.20 KB,
patch
|
446240525
:
review+
|
Details | Diff | Splinter Review |
Array.from({[Symbol.iterator]:0}) >typein:1:0 TypeError: usingIterator is not a function I'm going to fix this when we add GetMethod() in bug 896608.
Updated•9 years ago
|
Attachment #8549385 -
Flags: review?(till)
Comment 2•9 years ago
|
||
Comment on attachment 8549385 [details] [diff] [review] bug-1121391-v1.patch Review of attachment 8549385 [details] [diff] [review]: ----------------------------------------------------------------- Excellent, r=me with the one comment addressed. ::: js/src/builtin/Array.js @@ +762,4 @@ > } > > + // Step 7 is an assertion: items is not an Iterator. Testing this is > + // literally the very last thing we did, so we don't assert here. Ok, after filing https://bugs.ecmascript.org/show_bug.cgi?id=3525 and then marking it as invalid, I see that this still holds. However, the comment doesn't reflect reality anymore: we did just test this as far as actual control flow goes, but the reason for it being impossible to reach this point if `items` is an Iterator is more subtle. I think we should just add the assert to make it clear that this actually does hold. Dumb people like me might need a moment to understand why, but at least they'd be certain it does.
Attachment #8549385 -
Flags: review?(till) → review+
Attachment #8549385 -
Attachment is obsolete: true
Attachment #8549610 -
Flags: review?(till)
Comment 4•9 years ago
|
||
Comment on attachment 8549610 [details] [diff] [review] review comment addressed Review of attachment 8549610 [details] [diff] [review]: ----------------------------------------------------------------- Yup. No need to re-request a review if you have an "r+ with feedback addressed". (Yes, I did request a further change here, but landing without that change would've been ok.) ::: js/src/builtin/Array.js @@ +762,4 @@ > } > > + // Step 7. > + assert(usingIterator === undefined, "'items' could not be an iterable"); "`items` can't be an Iterator after step 6.g.iv"
Attachment #8549610 -
Flags: review?(till) → review+
Attachment #8549610 -
Attachment is obsolete: true
Attachment #8550173 -
Flags: review+
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/26263767b141
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/26263767b141
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•