[webkit-reviews] review denied: [Bug 12878] Support for each ( var in collection ) syntax : [Attachment 13368] Implements for each ( var <identifier> in <collection> )

bugzilla-request-daemon at macosforge.org bugzilla-request-daemon at macosforge.org
Sun Feb 25 17:51:49 PST 2007


Mark Rowe (bdash) <bdash at webkit.org> has denied Mark Rowe (bdash)
<bdash at webkit.org>'s request for review:
Bug 12878: Support for each ( var in collection ) syntax
http://bugs.webkit.org/show_bug.cgi?id=12878

Attachment 13368: Implements for each ( var <identifier> in <collection> )
http://bugs.webkit.org/attachment.cgi?id=13368&action=edit

------- Additional Comments from Mark Rowe (bdash) <bdash at webkit.org>
+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.



More information about the webkit-reviews mailing list