Bug 12878 - Support for each (var in collection) syntax
Summary: Support for each (var in collection) syntax
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC All
: P3 Normal
Assignee: Justin Haygood
URL: http://server250.reaktix.com/array_fo...
Keywords:
: 20728 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-02-24 10:50 PST by Justin Haygood
Modified: 2012-03-08 18:11 PST (History)
7 users (show)

See Also:


Attachments
mini test case (287 bytes, text/html)
2007-02-24 10:51 PST, Justin Haygood
no flags Details
Adds support for for..each..in (9.07 KB, patch)
2007-02-24 12:08 PST, Justin Haygood
ap: review-
Details | Formatted Diff | Diff
Implements for each ( var <identifier> in <collection> ) (8.75 KB, patch)
2007-02-25 10:34 PST, Justin Haygood
mrowe: review-
Details | Formatted Diff | Diff
Updated Patch (8.10 KB, patch)
2007-08-01 09:43 PDT, Justin Haygood
mrowe: review-
Details | Formatted Diff | Diff
for each...in test case (1.60 KB, application/zip)
2007-08-01 13:15 PDT, Justin Haygood
no flags Details
Updated Patch for for each...in (8.00 KB, patch)
2007-08-01 13:18 PDT, Justin Haygood
mrowe: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Haygood 2007-02-24 10:50:20 PST
JavaScript 1.6 supports for each ( var in collection ) syntax, like:

var array = ["foo","bar"];

for each ( var string in array )
{
document.write( string + "<br>" )
}

Should result in:

foo<br>
bar<br>

Currently, it gets an error.

This is one area where we can make the first effort at supporting E4X, but this is useful in any JavaScript/DOM collection as well.
Comment 1 Justin Haygood 2007-02-24 10:51:02 PST
Created attachment 13362 [details]
mini test case

Test case for bug. Works in Firefox, Fails elsewhere.
Comment 2 Justin Haygood 2007-02-24 12:08:06 PST
Created attachment 13364 [details]
Adds support for for..each..in

This is an initial implementation of for...each...in, supported by Mozilla Firefox currently. This also is required for support of E4X, but is useable elsewhere, such as iterating through arrays more naturally.
Comment 3 Alexey Proskuryakov 2007-02-24 12:41:05 PST
Comment on attachment 13364 [details]
Adds support for for..each..in

r- for purely stylistic issues discussed via IRC (star and comma positioning, assert vs. ASSERT). And clearly, this cannot be landed to trunk before the stabilization period ends.

A couple additional stylistic notes:

+        if (varDecl)
+    s << "var " << varDecl;

  Indentation looks broken here.

+            if ((c.complType() == Break) && ls.contains(c.target()))
+            break;

  And here.

+    JSValue* e;
...
+    e = expr->evaluate(exec);

  There is no need to declare local variables at the beginning of the function in C++. Also, this and other variables may benefit from a more descriptive name.
Comment 4 Justin Haygood 2007-02-25 10:34:03 PST
Created attachment 13368 [details]
Implements for each ( var <identifier> in <collection> )

Same as above mostly, except:
 * Removes unused variables
 * Follows code style guidelines
Comment 5 Mark Rowe (bdash) 2007-02-25 17:51:48 PST
Comment on attachment 13368 [details]
Implements for each ( var <identifier> in <collection> )

+void ForEachInNode::streamTo(SourceStream &s) const
+{
+    s << SourceStream::Endl << "for each (";
+        if (varDecl)
+    s << "var " << varDecl;
+        else
+    s << lexpr;
+

Indentation is incorrect here.

+            if ((c.complType() == Break) && ls.contains(c.target()))
+            break;

And again here.

The ForEachInNode constructor should set varDecl and lexpr in the initializer list, and the formatting of this list should match the coding style guidelines.

It looks as though varDecl is only used in streamTo, which means that "for each (a in b)" and "for each (var a in b)" are treated identically when they should have different semantics.

+        JSValue* str = v->get(exec, name);

"str" seems to be a bit misleading for the new use of this variable, I gather it's now the value of the property retrieved from the iterator.

It seems that almost all of the code in ForEachInNode::execute is duplicated from ForInNode which is less than ideal.  It's also not clear why supporting only a single feature from the E4X specification is desirable.
Comment 6 Mark Rowe (bdash) 2007-02-25 17:57:43 PST
Note that this feature comes from ECMA 357 (<http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-357.pdf>), the ECMAScript for XML specification.  You should verify that your implementation matches this specification and definitely include test cases to support this.
Comment 7 Justin Haygood 2007-08-01 09:43:12 PDT
Created attachment 15782 [details]
Updated Patch

1. Implemented by subclassing from ForInNode, so that it doesn't duplicate code effort
2. Testcases are on the way
3. Doesn't cause any regressions
4. This is an experimental patch. Questions/Concerns, etc.. are very much welcome!
Comment 8 Mark Rowe (bdash) 2007-08-01 10:25:04 PDT
Comment on attachment 15782 [details]
Updated Patch

I pointed out a few issues in the patch to Justin via IRC.  There are numerous coding style inconsistencies, and the redeclaration of ForInNode's instance variables in the subclass is wrong.  I also suggested that the implementation of streamTo could easily be shared between the two classes which should allow ForInNode's members to remain private.
Comment 9 Mark Rowe (bdash) 2007-08-01 10:25:56 PDT
Oh, and test cases!  Test cases, test cases, test cases! ;-)
Comment 10 Justin Haygood 2007-08-01 13:15:42 PDT
Created attachment 15790 [details]
for each...in test case

Testcase
Comment 11 Justin Haygood 2007-08-01 13:18:24 PDT
Created attachment 15791 [details]
Updated Patch for for each...in

This has been modified to follow code style guidelines.

It also merges streamTo between ForInNode and ForEachInNode and restores the "private" in ForInNode
Comment 12 Mark Rowe (bdash) 2007-08-01 20:32:50 PDT
Comment on attachment 15791 [details]
Updated Patch for for each...in

There is still incorrect whitespace and * placement in many places.  You should also include a test case as part of the patch, along with the related changelog entry.

+                                            Node *n = $4->nodeInsideAllParens();

+JSValue* ForInNode::getIteratorValue( ExecState*, JSObject* inValue, const Identifier name )

+      JSValue *str = getIteratorValue(exec,v,name);

+JSValue* ForEachInNode::getIteratorValue(ExecState* exec, JSObject* inValue, const Identifier name )

+    virtual JSValue* getIteratorValue( ExecState *exec, JSObject *inValue, const Identifier name ) KJS_FAST_CALL;


+    ForEachInNode(Node *l, Node *e, StatementNode *s) KJS_FAST_CALL;
+    ForEachInNode(const Identifier &i, AssignExprNode *in, Node *e, StatementNode *s) KJS_FAST_CALL;
+  protected:
+    virtual JSValue* getIteratorValue( ExecState *exec, JSObject *inValue, const Identifier name ) KJS_FAST_CALL;
Comment 13 Cameron Zwarich (cpst) 2008-09-08 09:13:06 PDT
*** Bug 20728 has been marked as a duplicate of this bug. ***
Comment 14 Gavin Barraclough 2012-03-08 18:11:54 PST
Our goal is ES-262 compliance, and I don't think it looks "for each" looks like it's going to make it into ES6.  As such, I don't think we plan to support this.  If "for each" makes it onto the standardization track, we should revisit this.