Closed Bug 1015742 Opened 7 years ago Closed 7 years ago
_SPREAD to work on iterator
Discussion on irc: jorendorff: Swatinem: change the existing JSOP_SPREAD bytecode to take an iterator, not an iterable jorendorff: Swatinem: change all our existing code that uses JSOP_SPREAD to first emit a JSOP_CALLPROP "@@iterator" jorendorff: Swatinem: then the new JSOP_SPREAD behavior will be exactly what rest-destructuring needs. What do you think? jorendorff: We may eventually want to remove JSOP_SPREAD, but that can happen later. Swatinem: hm ok, so in the spread case, jsop_spread would get a fresh iterator to fill into an already filled array Swatinem: and in the rest-destructuring, it will get a half-spent iterator to fill into an empty array? jorendorff: yep Swatinem: sounds good. Swatinem: i was looking at jsop_spread before, but it didnt quite do what was needed for my case jorendorff: right jorendorff: but it shouldn't be hard to fix that Swatinem: and incidentally, that would also take care of the bug that one cant use generic iterators with destructuring right now jorendorff: Well ... I kind of assumed you'd be doing that already. jorendorff: heh jorendorff: it needs to be done. a lot of tests will have to be fixed :-\
Wow, this was easier than expected. luckily GetIterator(iterator) === iterator; so I didn’t have to change ForOfIterator. Note: I haven’t run the testsuite yet, just tested it locally.
Assignee: nobody → arpad.borsos
Status: NEW → ASSIGNED
Attachment #8428827 - Flags: review?(jorendorff)
FAILURES: js/src/jit-test/tests/basic/spread-array.js js/src/jit-test/tests/basic/spread-call-eval.js js/src/jit-test/tests/basic/spread-call-funapply.js js/src/jit-test/tests/basic/spread-call-length.js js/src/jit-test/tests/basic/spread-call.js I will take a look at those tomorrow.
jit-test passes now locally, haven’t run it through try yet though.
https://tbpl.mozilla.org/?tree=Try&rev=0fe98a371e67 looks pretty green so far :-)
Comment on attachment 8429427 [details] [diff] [review] jsopspread.patch Review of attachment 8429427 [details] [diff] [review]: ----------------------------------------------------------------- Looks great.
Attachment #8429427 - Flags: review?(jorendorff) → review+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.