Added method map to Collection interface and all its implementations (except classes generated fr...

Added method map to Collection interface and all its implementations (except classes generated from IDL for the DOM).
Committed: https://code.google.com/p/dart/source/detail?r=3223

[gbracha, 2012-01-05 22:14:48]

I added a map method to collections.  IDL fixes wil be provided by SEA before
this is fully viable. Please check to see if all needed changed to JS compiler
builds were done (tests pass, but still ...).

[ahe, 2012-01-05 22:36:54]

LGTM!

Mostly nits, but there are a few things that are important.

Cheers,
Peter

http://codereview.chromium.org/9114021/diff/1/client/dom/frog/dom_frog.dart
File client/dom/frog/dom_frog.dart (right):

http://codereview.chromium.org/9114021/diff/1/client/dom/frog/dom_frog.dart#n...
client/dom/frog/dom_frog.dart:12017: List<Object> destination,
Indentation.

http://codereview.chromium.org/9114021/diff/1/client/dom/frog/dom_frog.dart#n...
client/dom/frog/dom_frog.dart:12018: f(o)) {
Indentation.

http://codereview.chromium.org/9114021/diff/1/client/dom/frog/dom_frog.dart#n...
client/dom/frog/dom_frog.dart:12027: bool f(o)) {
Please follow this style.

http://codereview.chromium.org/9114021/diff/1/client/dom/src/_Collections.dart
File client/dom/src/_Collections.dart (right):

http://codereview.chromium.org/9114021/diff/1/client/dom/src/_Collections.dar...
client/dom/src/_Collections.dart:18: List<Object> destination,
Indentation.

http://codereview.chromium.org/9114021/diff/1/client/dom/src/_Collections.dar...
client/dom/src/_Collections.dart:19: bool f(o)) {
Indentation.

http://codereview.chromium.org/9114021/diff/1/client/html/src/ElementWrapping...
File client/html/src/ElementWrappingImplementation.dart (right):

http://codereview.chromium.org/9114021/diff/1/client/html/src/ElementWrapping...
client/html/src/ElementWrappingImplementation.dart:172: throw 'Not impl yet.
todo(jacobr)';
Please don't abbreviate and follow TODO conventions:

// TODO(jacobr): Implement this.
throw 'not implemented yet';

http://codereview.chromium.org/9114021/diff/1/client/html/src/_Collections.dart
File client/html/src/_Collections.dart (right):

http://codereview.chromium.org/9114021/diff/1/client/html/src/_Collections.da...
client/html/src/_Collections.dart:18: List<Object> destination,
Indentation

http://codereview.chromium.org/9114021/diff/1/client/html/src/_Collections.da...
client/html/src/_Collections.dart:19: bool f(o)) {
Indentation.

http://codereview.chromium.org/9114021/diff/1/client/samples/swarm/DataSource...
File client/samples/swarm/DataSource.dart (right):

http://codereview.chromium.org/9114021/diff/1/client/samples/swarm/DataSource...
client/samples/swarm/DataSource.dart:91: return Collections.map(this, new
List<Section>(), f);
How do you know that f returns a Section?

http://codereview.chromium.org/9114021/diff/1/compiler/lib/implementation/col...
File compiler/lib/implementation/collections.dart (right):

http://codereview.chromium.org/9114021/diff/1/compiler/lib/implementation/col...
compiler/lib/implementation/collections.dart:18: List<Object> destination,
Indentation.

http://codereview.chromium.org/9114021/diff/1/compiler/lib/implementation/col...
compiler/lib/implementation/collections.dart:19: bool f(o)) {
Indentation.

http://codereview.chromium.org/9114021/diff/1/corelib/src/collection.dart
File corelib/src/collection.dart (right):

http://codereview.chromium.org/9114021/diff/1/corelib/src/collection.dart#new...
corelib/src/collection.dart:21: * type would be Collection<R>.
This comment is confusing. The static type may conceptually be Collection<R>,
but the runtime type will always be Collection.

http://codereview.chromium.org/9114021/diff/1/corelib/src/collection.dart#new...
corelib/src/collection.dart:33: 
Extra line.

http://codereview.chromium.org/9114021/diff/1/corelib/src/implementation/queu...
File corelib/src/implementation/queue.dart (right):

http://codereview.chromium.org/9114021/diff/1/corelib/src/implementation/queu...
corelib/src/implementation/queue.dart:209: Queue<E> other = new Queue();
Remove <E>.

http://codereview.chromium.org/9114021/diff/1/frog/lib/collections.dart
File frog/lib/collections.dart (right):

http://codereview.chromium.org/9114021/diff/1/frog/lib/collections.dart#newco...
frog/lib/collections.dart:18: List<Object> destination,
Indentation.

http://codereview.chromium.org/9114021/diff/1/frog/lib/collections.dart#newco...
frog/lib/collections.dart:19: bool f(o)) {
Indentation.

http://codereview.chromium.org/9114021/diff/1/frog/lib/corelib_impl.dart
File frog/lib/corelib_impl.dart (right):

http://codereview.chromium.org/9114021/diff/1/frog/lib/corelib_impl.dart#newc...
frog/lib/corelib_impl.dart:93: Collection map(f(E element)) native;
Sigh.

http://codereview.chromium.org/9114021/diff/1/runtime/lib/array.dart
File runtime/lib/array.dart (right):

http://codereview.chromium.org/9114021/diff/1/runtime/lib/array.dart#newcode88
runtime/lib/array.dart:88: return Collections.map(this, new
GrowableObjectArray(), f);
new GrowableObjectArray(length)

http://codereview.chromium.org/9114021/diff/1/runtime/lib/byte_buffer.dart
File runtime/lib/byte_buffer.dart (right):

http://codereview.chromium.org/9114021/diff/1/runtime/lib/byte_buffer.dart#ne...
runtime/lib/byte_buffer.dart:97: return Collections.map(this, new
GrowableObjectArray(), f);
new GrowableObjectArray(length)

http://codereview.chromium.org/9114021/diff/1/runtime/lib/collections.dart
File runtime/lib/collections.dart (right):

http://codereview.chromium.org/9114021/diff/1/runtime/lib/collections.dart#ne...
runtime/lib/collections.dart:32: List<Object> destination,
Indentation.

http://codereview.chromium.org/9114021/diff/1/runtime/lib/collections.dart#ne...
runtime/lib/collections.dart:33: bool f(Object o)) {
Indentation.

http://codereview.chromium.org/9114021/diff/1/runtime/lib/growable_array.dart
File runtime/lib/growable_array.dart (right):

http://codereview.chromium.org/9114021/diff/1/runtime/lib/growable_array.dart...
runtime/lib/growable_array.dart:202: return Collections.map(this, new
GrowableObjectArray(), f);
new GrowableObjectArray(length)

http://codereview.chromium.org/9114021/diff/1/tests/corelib/src/QueueTest.dart
File tests/corelib/src/QueueTest.dart (right):

http://codereview.chromium.org/9114021/diff/1/tests/corelib/src/QueueTest.dar...
tests/corelib/src/QueueTest.dart:38: return value~/10;
Spaces around operator.

http://codereview.chromium.org/9114021/diff/1/tests/corelib/src/QueueTest.dar...
tests/corelib/src/QueueTest.dart:41: bool is10(int value) {
Remove trailing whitespace.

[sra1, 2012-01-05 22:46:48]

I'll start be working on the DOM side after finishing up my current task.

http://codereview.chromium.org/9114021/diff/1/client/dom/frog/dom_frog.dart
File client/dom/frog/dom_frog.dart (right):

http://codereview.chromium.org/9114021/diff/1/client/dom/frog/dom_frog.dart#n...
client/dom/frog/dom_frog.dart:12017: List<Object> destination,
On 2012/01/05 22:36:54, ahe wrote:
> Indentation.

This file is generated, don't bother fixing here, but fix at the source (any
other file)

http://codereview.chromium.org/9114021/diff/1/client/html/src/_Collections.dart
File client/html/src/_Collections.dart (right):

http://codereview.chromium.org/9114021/diff/1/client/html/src/_Collections.da...
client/html/src/_Collections.dart:17: static List map(Iterable<Object> source,
Question - is there a good reason for this to be Iterable<Object> rather than
just Iterable (aka Iterable/*<Dynamic>*/)

http://codereview.chromium.org/9114021/diff/1/client/html/src/_Collections.da...
client/html/src/_Collections.dart:18: List<Object> destination,
Indentation - make these line up

http://codereview.chromium.org/9114021/diff/1/frog/utils.dart
File frog/utils.dart (right):

http://codereview.chromium.org/9114021/diff/1/frog/utils.dart#newcode125
frog/utils.dart:125: Collection map(f(E element)) {
nit: prefer =>, especially if it fits on one line.

http://codereview.chromium.org/9114021/diff/1/tests/corelib/src/RegExpAllMatc...
File tests/corelib/src/RegExpAllMatchesTest.dart (right):

http://codereview.chromium.org/9114021/diff/1/tests/corelib/src/RegExpAllMatc...
tests/corelib/src/RegExpAllMatchesTest.dart:43: var mapped = matches.map((Match
m) {
nit. could use  matches.map((Match m) => m.group(0) + "bar")

[Ivan Posva, 2012-01-05 22:47:02]

LGTM (from the VM perspective).

-Ivan

http://codereview.chromium.org/9114021/diff/1/corelib/src/collection.dart
File corelib/src/collection.dart (right):

http://codereview.chromium.org/9114021/diff/1/corelib/src/collection.dart#new...
corelib/src/collection.dart:21: * type would be Collection<R>.
I don't think this comment is entirely correct. The return type is probably
Collection<Dynamic>, no? I don't think there is a way to construct Collection<R>
based on the function alone.

http://codereview.chromium.org/9114021/diff/1/frog/lib/collections.dart
File frog/lib/collections.dart (right):

http://codereview.chromium.org/9114021/diff/1/frog/lib/collections.dart#newco...
frog/lib/collections.dart:18: List<Object> destination,
Please fix the indentation.

http://codereview.chromium.org/9114021/diff/1/runtime/lib/array.dart
File runtime/lib/array.dart (right):

http://codereview.chromium.org/9114021/diff/1/runtime/lib/array.dart#newcode88
runtime/lib/array.dart:88: return Collections.map(this, new
GrowableObjectArray(), f);
Maybe even allocate a new ObjectArray(length), but this is a bigger question
about what types the collection methods return.

[Jennifer Messerly, 2012-01-05 22:54:20]

few comments but overall lgtm

http://codereview.chromium.org/9114021/diff/1/client/html/src/ElementWrapping...
File client/html/src/ElementWrappingImplementation.dart (right):

http://codereview.chromium.org/9114021/diff/1/client/html/src/ElementWrapping...
client/html/src/ElementWrappingImplementation.dart:34: List<Element> output =
new List<Element>();
Is this right? It seems like the type should be List, e.g.:
    List output = [];

http://codereview.chromium.org/9114021/diff/1/client/html/src/ElementWrapping...
client/html/src/ElementWrappingImplementation.dart:36: output.add(f(element));
nit: indent -2

http://codereview.chromium.org/9114021/diff/1/client/html/src/ElementWrapping...
client/html/src/ElementWrappingImplementation.dart:172: throw 'Not impl yet.
todo(jacobr)';
On 2012/01/05 22:36:54, ahe wrote:
> Please don't abbreviate and follow TODO conventions:
> 
> // TODO(jacobr): Implement this.
> throw 'not implemented yet';

could also be "throw const NotImplementedException();" ?

(although now that I look further down, it seems like the existing code was
throwing strings too.)

http://codereview.chromium.org/9114021/diff/1/client/html/src/NodeWrappingImp...
File client/html/src/NodeWrappingImplementation.dart (right):

http://codereview.chromium.org/9114021/diff/1/client/html/src/NodeWrappingImp...
client/html/src/NodeWrappingImplementation.dart:29: List<Node> output = new
List<Node>();
similar comment, I think this should be "List output = [];" since the return
type of "f" is arbitrary

http://codereview.chromium.org/9114021/diff/1/compiler/lib/implementation/arr...
File compiler/lib/implementation/array.dart (right):

http://codereview.chromium.org/9114021/diff/1/compiler/lib/implementation/arr...
compiler/lib/implementation/array.dart:80: return Collections.map(this, new
List(), f);
minor style comment, I'd just use "[]" here instead of "new List()"

[vsm, 2012-01-05 23:53:42]

LGTM for Dartium (but please wait for sra on DartC and Frog DOM)

[zundel, 2012-01-06 14:42:11]

http://codereview.chromium.org/9114021/diff/1/compiler/lib/implementation/col...
File compiler/lib/implementation/collections.dart (right):

http://codereview.chromium.org/9114021/diff/1/compiler/lib/implementation/col...
compiler/lib/implementation/collections.dart:21: destination.add(f(e));
I don't understand what this is supposed to do.  It looks like, according to the
method signature, that it will call the function 'f' and populate a list with
values 'true' or 'false' in each position.

[floitsch, 2012-01-10 13:45:19]

LGTM, but the next time please include Nicolas and/or me when doing core-library
changes (and probably Josh too).
The map function was left out on purpose and we intended to discuss different
approaches with Josh (one of which is the one you now implemented).

[gbracha, 2012-01-12 00:53:57]

Ok, all comments have been acted upon. Are we good to go?

http://codereview.chromium.org/9114021/diff/1/client/html/src/_Collections.dart
File client/html/src/_Collections.dart (right):

http://codereview.chromium.org/9114021/diff/1/client/html/src/_Collections.da...
client/html/src/_Collections.dart:17: static List map(Iterable<Object> source,
On 2012/01/05 22:46:49, sra1 wrote:
> Question - is there a good reason for this to be Iterable<Object> rather than
> just Iterable (aka Iterable/*<Dynamic>*/)

I don't think so. I've followed the style that's already in place in this class.

http://codereview.chromium.org/9114021/diff/1/client/samples/swarm/DataSource...
File client/samples/swarm/DataSource.dart (right):

http://codereview.chromium.org/9114021/diff/1/client/samples/swarm/DataSource...
client/samples/swarm/DataSource.dart:91: return Collections.map(this, new
List<Section>(), f);
On 2012/01/05 22:36:54, ahe wrote:
> How do you know that f returns a Section?

I don't. It's a mistake. Fixed.

http://codereview.chromium.org/9114021/diff/1/compiler/lib/implementation/arr...
File compiler/lib/implementation/array.dart (right):

http://codereview.chromium.org/9114021/diff/1/compiler/lib/implementation/arr...
compiler/lib/implementation/array.dart:80: return Collections.map(this, new
List(), f);
On 2012/01/05 22:54:21, John Messerly wrote:
> minor style comment, I'd just use "[]" here instead of "new List()"

I'm of two minds on that. I agree it looks nice, but to me [] feels like an
immutable empty list, so I tend to avoid it in  cases like these.

http://codereview.chromium.org/9114021/diff/1/compiler/lib/implementation/col...
File compiler/lib/implementation/collections.dart (right):

http://codereview.chromium.org/9114021/diff/1/compiler/lib/implementation/col...
compiler/lib/implementation/collections.dart:21: destination.add(f(e));
On 2012/01/06 14:42:12, zundel wrote:
> I don't understand what this is supposed to do.  It looks like, according to
the
> method signature, that it will call the function 'f' and populate a list with
> values 'true' or 'false' in each position.

Oops, cut-and-paste error. Obviously, the function need not be boolean. Fixed.

http://codereview.chromium.org/9114021/diff/1/runtime/lib/array.dart
File runtime/lib/array.dart (right):

http://codereview.chromium.org/9114021/diff/1/runtime/lib/array.dart#newcode88
runtime/lib/array.dart:88: return Collections.map(this, new
GrowableObjectArray(), f);
On 2012/01/05 22:47:03, Ivan Posva wrote:
> Maybe even allocate a new ObjectArray(length), but this is a bigger question
> about what types the collection methods return.

I suppose someone could compute the mapped collection and then want to add stuff
to it. Bad style perhaps, but I leave it to the library designer to decide.