1388317 - Remove E4X support for E4X for-each

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tom S [:evilpie]]

Bug 1083470 just disabled for-each, but didn't actually remove any code for it in the parser, interpreter etc.

Comment 1

[Masatoshi Kimura [:emk]]

Yeah, I intentionally left the code just in case. We could remove E4X for-each support completely when bug 1083470 reaches to release.

Comment 2

[Tom S [:evilpie]]

Attachment: WIP

I think we could land the removal now. I started working on this a while back, but there is definitely a lot of auxiliary stuff that be removed, like e.g. the flags parameter to JSOP_ITER.

Comment 3

[Jason Orendorff [:jorendorff]]

Oh, I have a patch for this too, very similar of course.

Let's do it.

Comment 4

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed1f28410003f1f14c487e4b5d75b8cf2089a7d2

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Remove support for E4X `for each` loop

Comment 6

[Tom S [:evilpie]]

Comment on attachment 8925955 [details] [diff] [review]
Remove support for E4X `for each` loop

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7473,5 @@
>  
>      // Convert the value to the appropriate sort of iterator object for the
>      // loop variant (for-in, for-each-in, or destructuring for-in).
>      unsigned iflags = forInLoop->pn_iflags;
> +    MOZ_ASSERT(0 == (iflags & ~JSITER_ENUMERATE));

I think we can just assert iflags == JSITER_ENUMERATE at the top of the function. Or at least the comment needs to be adjusted.

::: js/src/jit-test/tests/basic/bug642772-3.js
@@ +1,4 @@
>  // Catch memory leaks when enumerating over the global object.
>  
>  for (let z = 1; z <= 16000; ++z) {
> +  for (y in this);

I think we should add this[y] if we want to keep this test.

::: js/src/jit-test/tests/basic/testStringResolve.js
@@ -1,5 @@
> -// |jit-test| need-for-each
> -
> -function testStringResolve() {
> -    var x = 0;
> -    for each (let d in [new String('q'), new String('q'), new String('q')]) {

I would keep this test with for-of.

::: js/src/tests/js1_8_5/shell.js
@@ -24,5 @@
>  }
>  
> -// NOTE: This only turns on 1.8.5 in shell builds.  The browser requires the
> -//       futzing in js/src/tests/browser.js (which only turns on 1.8, the most
> -//       the browser supports).

Was this comment incorrect?

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8925955 [details] [diff] [review]
Remove support for E4X `for each` loop

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

::: js/src/tests/js1_8_5/shell.js
@@ -24,5 @@
>  }
>  
> -// NOTE: This only turns on 1.8.5 in shell builds.  The browser requires the
> -//       futzing in js/src/tests/browser.js (which only turns on 1.8, the most
> -//       the browser supports).

I believe I wrote this comment at a time when let required a JS version that supported it.  Now that let's enabled everywhere, with the goofy quasi-backwards-compatible syntax, the comment is likely dated.

Comment 8

[Jeff Walden [:Waldo]]

Well, actually.  No, that's wrong.  The comment is still entirely accurate that this only enables version 185 in shell tests.  The only way to get JS tests run as 185 in the browser, is to have the type accordingly modified when the <script> tags are appended to the DOM.  See the code in jsTestDriverBrowserInit in browser.js, as the comment noted.

So I guess I don't know why that comment is being removed, because it's still accurate.

But if tests don't need version(185) in the shell any more, then the version-set should be removed entirely, both here *and* in browser.js.

Comment 9

[Jason Orendorff [:jorendorff]]

I put the comment back in. I removed it because I misread the comment. I thought it was referring to browser.js *in that directory*, which happens to be empty in js1_8_5.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92921ec579f6
Remove support for E4X `for each` loop. r=evilpie.

Comment 11

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92921ec579f6dcc936c2614e4d5190e29ece0170
Bug 1388317 - Remove support for E4X `for each` loop. r=evilpie.

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/92921ec579f6