refactor JSOP_SPREAD to work on iterator

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Swatinem, Assigned: Swatinem)

Tracking

(Blocks 1 bug)

unspecified
mozilla32
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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 :-\
No longer blocks: spread-assignment
Posted patch jsopspread.patch (obsolete) — Splinter Review
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.
Attachment #8428827 - Attachment is obsolete: true
Attachment #8428827 - Flags: review?(jorendorff)
Attachment #8429427 - Flags: review?(jorendorff)
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+
https://hg.mozilla.org/mozilla-central/rev/310d82551d3b
https://hg.mozilla.org/mozilla-central/rev/7371145721bf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.