Closed Bug 1388317 Opened 3 years ago Closed 3 years ago

Remove E4X support for E4X for-each

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: evilpie, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(Whiteboard: [js:tech-debt])

Attachments

(1 file, 1 obsolete file)

Bug 1083470 just disabled for-each, but didn't actually remove any code for it in the parser, interpreter etc.
Yeah, I intentionally left the code just in case. We could remove E4X for-each support completely when bug 1083470 reaches to release.
Priority: -- → P3
Whiteboard: [js:tech-debt]
Assignee: nobody → evilpies
Assignee: evilpies → nobody
Attached patch WIP (obsolete) — Splinter Review
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.
Blocks: 1412532
Oh, I have a patch for this too, very similar of course.

Let's do it.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Attachment #8921595 - Attachment is obsolete: true
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?
Attachment #8925955 - Flags: review?(evilpies) → review+
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.
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.
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.
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92921ec579f6
Remove support for E4X `for each` loop. r=evilpie.
https://hg.mozilla.org/mozilla-central/rev/92921ec579f6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 1420961
You need to log in before you can comment on or make changes to this bug.