teramako Reporter
	Description • 12 years ago

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

	Yusuke Suzuki
	Comment 1 • 12 years ago

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

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 2 • 12 years ago

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.

	Yusuke Suzuki
	Comment 3 • 12 years ago

> I've already done it this time.

Thanks for your help!

	:Benjamin Peterson
	Comment 4 • 12 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.

	Yusuke Suzuki
	Comment 5 • 12 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.

	:Benjamin Peterson
	Comment 6 • 12 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

	Yusuke Suzuki
	Comment 7 • 12 years ago

Attached patch rev2

	Yusuke Suzuki
	Comment 8 • 12 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.

	:Benjamin Peterson
	Comment 9 • 12 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 10 • 12 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 11 • 12 years ago

Isn't WONTFIX better than INVALID for this bug?

	teramako Reporter
	Comment 12 • 12 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

	Tetsuharu OHZEKI [:tetsuharu] (UTC+9)
	Comment 13 • 12 years ago

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

	Ben Bucksch (:BenB)
	Comment 14 • 12 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.