1015742 - refactor JSOP_SPREAD to work on iterator

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Arpad Borsos [:Swatinem]]

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 :-\

Comment 1

[Arpad Borsos [:Swatinem]]

Attachment: jsopspread.patch

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.

Comment 2

[Arpad Borsos [:Swatinem]]

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.

Comment 3

[Arpad Borsos [:Swatinem]]

Attachment: jsopspread.patch

jit-test passes now locally, haven’t run it through try yet though.

Comment 4

[Arpad Borsos [:Swatinem]]

https://tbpl.mozilla.org/?tree=Try&rev=0fe98a371e67 looks pretty green so far :-)

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8429427 [details] [diff] [review]
jsopspread.patch

Review of attachment 8429427 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great.

Comment 6

[Arpad Borsos [:Swatinem]]

Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/310d82551d3b

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/310d82551d3b
https://hg.mozilla.org/mozilla-central/rev/7371145721bf