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

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[teramako]

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

Comment 1

[Yusuke Suzuki]

Attachment: rev1, insert ifdef and guard to for-each statement

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

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 3

[Yusuke Suzuki]

> I've already done it this time.

Thanks for your help!

Comment 4

[:Benjamin Peterson]

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

[Yusuke Suzuki]

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

[:Benjamin Peterson]

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

[Yusuke Suzuki]

Attachment: rev2 patch

Attached patch rev2

Comment 8

[Yusuke Suzuki]

> 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

[:Benjamin Peterson]

That transition is not going to be complete before FF 18.

Comment 10

[Jason Orendorff [:jorendorff]]

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.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Isn't WONTFIX better than INVALID for this bug?

Comment 12

[teramako]

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

Comment 13

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(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.

Comment 14

[Ben Bucksch (:BenB)]

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.