Closed Bug 791343 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: teramako, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

ECMA-357(E4X) is deprecated.
But for-each statement is still available even if javascript.options.xml.{content|chrome} is false.
Blocks: 788290
Version: 13 Branch → Trunk
In the meantime, I've created rev1 patch inserting if-def and guard.
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)
> I've already done it this time.

Thanks for your help!
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.
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.
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
Attached patch rev2 patchSplinter Review
Attached patch rev2
Attachment #661337 - Attachment is obsolete: true
Attachment #661337 - Flags: review?(jorendorff)
Attachment #662291 - Flags: review?(benjamin)
> 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.
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
Closed: 8 years ago
Resolution: --- → INVALID
Isn't WONTFIX better than INVALID for this bug?
Resolution: INVALID → WONTFIX
Attachment #662291 - Flags: review?(benjamin)
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.
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.