disable for-each statement on javascript.options.xml.{content|chrome} = false

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
5 years ago
4 years ago

People

(Reporter: teramako, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
ECMA-357(E4X) is deprecated.
But for-each statement is still available even if javascript.options.xml.{content|chrome} is false.
(Reporter)

Updated

5 years ago
Blocks: 788290
Version: 13 Branch → Trunk

Comment 1

5 years ago
Created attachment 661337 [details] [diff] [review]
rev1, insert ifdef and guard to for-each statement

In the meantime, I've created rev1 patch inserting if-def and guard.
(Reporter)

Updated

5 years ago
Depends on: 791348
Comment on attachment 661337 [details] [diff] [review]
rev1, insert ifdef and guard to for-each statement

Yusuke, thanks for the patch.

Requesting for review from jorendorff on behalf. Please feel free to set the flag to review? and input the requestee, in this case jorendorff. I've already done it this time.
Attachment #661337 - Flags: review?(jorendorff)

Comment 3

5 years ago
> I've already done it this time.

Thanks for your help!

Comment 4

5 years ago
Comment on attachment 661337 [details] [diff] [review]
rev1, insert ifdef and guard to for-each statement

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

::: js/src/frontend/Parser.cpp
@@ +2978,2 @@
>      if (tokenStream.matchToken(TOK_NAME)) {
> +        if (tokenStream.currentToken().name() == context->runtime->atomState.eachAtom && allowsXML())

Put the |allowsXML()| as close to the #ifdef as possible. In this case:

    if (allowsXML() && tokenStream.matchToken(TOK_NAME))
        ...

@@ +5163,2 @@
>          if (tokenStream.matchToken(TOK_NAME)) {
> +            if (tokenStream.currentToken().name() == context->runtime->atomState.eachAtom && allowsXML())

Same as above.

Comment 5

5 years ago
Thanks for your review, Benjamin.

> Put the |allowsXML()| as close to the #ifdef as possible. In this case:

I've got it. I'll attach revised patch soon.

Comment 6

5 years ago
Also, I don't think this patch will fly. "for each" is a feature widely used in the browser [1] and has largely been separated from its E4X roots. It's not going away anytime soon.

[1] http://mxr.mozilla.org/mozilla-central/search?string=for+each&find=\.js&findi=\.js&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 7

5 years ago
Created attachment 662291 [details] [diff] [review]
rev2 patch

Attached patch rev2
Attachment #661337 - Attachment is obsolete: true
Attachment #661337 - Flags: review?(jorendorff)
Attachment #662291 - Flags: review?(benjamin)

Comment 8

5 years ago
> Also, I don't think this patch will fly.

Personally I think replacing for-each to for-of in mozilla code base gradually is better and finally we can apply this patch to SpiderMonkey.

Comment 9

5 years ago
That transition is not going to be complete before FF 18.
Yusuke, thanks for taking the time to look into this.

I don't think we will ever be able to remove 'for each'. I think it's a bad language feature, but removing it would break too many add-ons.

Users are right to be irritated when we break code they've been using successfully for years.  Removing features from Firefox is therefore something we do as a last resort, only when there is a clear overriding benefit.  We're removing the rest of E4X because the maintenance burden is out of all proportion to the few uses of E4X in tests and add-ons.  The maintenance burden of 'for each' is not as bad, and a lot more code uses it.

Closing this bug.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
Isn't WONTFIX better than INVALID for this bug?

Updated

5 years ago
Resolution: INVALID → WONTFIX

Updated

5 years ago
Attachment #662291 - Flags: review?(benjamin)
(Reporter)

Comment 12

5 years ago
ok, I give up removing for-each statement.
but I think should throw a warning message as "for-each statement is obsolete" when the statement is used
(In reply to teramako from comment #12)
> ok, I give up removing for-each statement.
> but I think should throw a warning message as "for-each statement is
> obsolete" when the statement is used

OR/And I think that we should indicate that for-each statement is "deprecated" in docs.
Depends on: 825801

Comment 14

4 years ago
Before removing for..each entirely, please consider:
"for..each" is very useful, makes the code a lot clearer.

for each (var item in array)
vs.
for (var item of array)
vs.
for (var i = 0; i < array.length; i++) {
  var item = array[i];
}

Unfortunately, for..of can't replace for..each. For example, when iterating over a plain object
var obj = { foo: 1, bar: 2}:
for each (var value in obj)
vs.
for (var [, value] of Iterator(obj))
for..of is nowhere near as clear to read (most important), nor convenient to type.

My plea: Before removing for..each, make sure that there is a replacement that is just as (or more) capable, clear/lean and convenient. Code readability has uttermost importance for me.
You need to log in before you can comment on or make changes to this bug.