Last Comment Bug 791343 - disable for-each statement on javascript.options.xml.{content|chrome} = false
: disable for-each statement on javascript.options.xml.{content|chrome} = false
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 791348 825801
Blocks: 788290
  Show dependency treegraph
 
Reported: 2012-09-14 13:01 PDT by teramako
Modified: 2013-02-04 23:46 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rev1, insert ifdef and guard to for-each statement (1.87 KB, patch)
2012-09-14 13:10 PDT, Yusuke Suzuki
no flags Details | Diff | Splinter Review
rev2 patch (1.79 KB, patch)
2012-09-18 13:31 PDT, Yusuke Suzuki
no flags Details | Diff | Splinter Review

Description teramako 2012-09-14 13:01:21 PDT
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 2012-09-14 13:10:56 PDT
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.
Comment 2 Gary Kwong [:gkw] [:nth10sd] 2012-09-14 13:24:24 PDT
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 2012-09-14 13:26:00 PDT
> I've already done it this time.

Thanks for your help!
Comment 4 :Benjamin Peterson 2012-09-18 13:22:15 PDT
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 2012-09-18 13:25:23 PDT
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 2012-09-18 13:26:40 PDT
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 2012-09-18 13:31:46 PDT
Created attachment 662291 [details] [diff] [review]
rev2 patch

Attached patch rev2
Comment 8 Yusuke Suzuki 2012-09-18 13:36:10 PDT
> 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 2012-09-18 13:40:00 PDT
That transition is not going to be complete before FF 18.
Comment 10 Jason Orendorff [:jorendorff] 2012-09-18 17:19:58 PDT
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] (Mozilla Japan) 2012-09-18 18:04:25 PDT
Isn't WONTFIX better than INVALID for this bug?
Comment 12 teramako 2012-09-18 19:33:24 PDT
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] 2012-09-18 22:16:16 PDT
(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) 2013-02-04 23:46:28 PST
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.

Note You need to log in before you can comment on or make changes to this bug.