Remove E4X support for E4X for-each

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: evilpie, Assigned: jorendorff)

Tracking

(Blocks 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox59 fixed)

Details

(Whiteboard: [js:tech-debt])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
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]
Reporter

Updated

2 years ago
Assignee: nobody → evilpies
Reporter

Updated

2 years ago
Assignee: evilpies → nobody
Reporter

Comment 2

2 years ago
Posted 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
Assignee

Comment 3

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

Let's do it.
Assignee

Updated

2 years ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Reporter

Updated

2 years ago
Attachment #8921595 - Attachment is obsolete: true
Reporter

Comment 6

2 years ago
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.
Assignee

Comment 9

2 years ago
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

2 years ago
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92921ec579f6
Remove support for E4X `for each` loop. r=evilpie.

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/92921ec579f6
Status: ASSIGNED → RESOLVED
Closed: 2 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.