Add Map constructors taking iterables.

Add Map constructors taking iterables.

BUG= http://dartbug.com/11330
R=floitsch@google.com

Committed: https://code.google.com/p/dart/source/detail?r=24514

[zarah, 2013-06-27 10:26:05]

Ptal

[floitsch, 2013-06-27 11:45:04]

LGTM.

Added Lasse in case he has comments on the dart-doc.

https://codereview.chromium.org/18023003/diff/1/sdk/lib/collection/hash_map.dart
File sdk/lib/collection/hash_map.dart (right):

https://codereview.chromium.org/18023003/diff/1/sdk/lib/collection/hash_map.d...
sdk/lib/collection/hash_map.dart:30: var keyFun = (key != null) ? key : (x) =>
x;
In "dart:" libraries we type local variables too. (yes this is not following the
style guide).

reuse the argument. This way you don't need to come up with a new name.
if (key == null) key = ...;

Use a static identity function. Otherwise the VM will probably allocate a new
function at every constructor call. (In theory it could eventually optimize this
away, but it won't happen in the near future).

https://codereview.chromium.org/18023003/diff/1/sdk/lib/collection/hash_map.d...
sdk/lib/collection/hash_map.dart:33: iterable.forEach((e) {
personally I prefer

for (var element in iterable) {
  map[key(element)] = value(element);
}

but that's just, because I expect it to be easier to get optimized.

https://codereview.chromium.org/18023003/diff/1/sdk/lib/collection/hash_map.d...
sdk/lib/collection/hash_map.dart:43: var keyIterator = keys.iterator;
Use type.

https://codereview.chromium.org/18023003/diff/1/sdk/lib/collection/hash_map.d...
sdk/lib/collection/hash_map.dart:55: if (hasNextKey != hasNextValue) {
if (hasNextKey || hasNextValue) {

It has the same effect, but, to me, it makes it more clear what happens.
hasNextKey != hasNextValue could (in if implemented naively) be more efficient,
though.

https://codereview.chromium.org/18023003/diff/1/sdk/lib/core/map.dart
File sdk/lib/core/map.dart (right):

https://codereview.chromium.org/18023003/diff/1/sdk/lib/core/map.dart#newcode23
sdk/lib/core/map.dart:23: * Creates a map where each key is an element from
[keys] with
Try a shorter first sentence. More details in the next paragraph.

Creates a [Map] associating the given [keys] to [values].

This constructor iterates over [keys] and [values] and maps each element of
[keys] to the corresponding element of [values].

If [keys] contains the same object multiple times, the last occurrence
overwrites the previous value.

It is an error ...

https://codereview.chromium.org/18023003/diff/1/sdk/lib/core/map.dart#newcode26
sdk/lib/core/map.dart:26: * The length of [keys] and [values] must be the same,
otherwise an ArgumentError
long line.

We usually don't say what kind of error happens:

"It is an error if the two [Iterable]s don't have the same length".

https://codereview.chromium.org/18023003/diff/1/sdk/lib/core/map.dart#newcode33
sdk/lib/core/map.dart:33: * Creates a map where each key-value pair is computed
from each element in
Creates a [Map] where the keys and values are computed from the [iterable].
// new line to have a separate paragraph in the doc.
For each element of the [iterable] this constructor computes a key/value pair,
by applying [key] and [value] respectively.

The keys of the key/value pairs do not need to be unique. The last occurrence of
a key will simply overwrite any previous value.

If no values ...

https://codereview.chromium.org/18023003/diff/1/tests/corelib/map_from_iterab...
File tests/corelib/map_from_iterable_test.dart (right):

https://codereview.chromium.org/18023003/diff/1/tests/corelib/map_from_iterab...
tests/corelib/map_from_iterable_test.dart:52: print(map.length);
debug print.

https://codereview.chromium.org/18023003/diff/1/tests/corelib/map_from_iterab...
tests/corelib/map_from_iterable_test.dart:103: }
Add test that the returned map is of the right generic type.

var map = new Map<int, String>.fromIterable(...);
Expect.isTrue(map is Map<int, String>);

// Make sure it is not just Map<dynamic, dynamic>.
Expect.isFalse(map is Map<String, dynamic>);
Expect.isFalse(map is Map<dynamic, int>);

https://codereview.chromium.org/18023003/diff/1/tests/corelib/map_from_iterab...
File tests/corelib/map_from_iterables_test.dart (right):

https://codereview.chromium.org/18023003/diff/1/tests/corelib/map_from_iterab...
tests/corelib/map_from_iterables_test.dart:17: var map = new
Map.fromIterables([1, 2, 3], ["one", "two", "three"]);
write at least one test with generic types.

https://codereview.chromium.org/18023003/diff/1/tests/corelib/map_from_iterab...
tests/corelib/map_from_iterables_test.dart:48: void identicalElementsTest() {
equalElementsTest (although they are identical here)

[zarah, 2013-06-27 13:02:43]

TBR

https://codereview.chromium.org/18023003/diff/1/sdk/lib/collection/hash_map.dart
File sdk/lib/collection/hash_map.dart (right):

https://codereview.chromium.org/18023003/diff/1/sdk/lib/collection/hash_map.d...
sdk/lib/collection/hash_map.dart:30: var keyFun = (key != null) ? key : (x) =>
x;
On 2013/06/27 11:45:04, floitsch wrote:
> In "dart:" libraries we type local variables too. (yes this is not following
the
> style guide).
> 
> reuse the argument. This way you don't need to come up with a new name.
> if (key == null) key = ...;
> 
> Use a static identity function. Otherwise the VM will probably allocate a new
> function at every constructor call. (In theory it could eventually optimize
this
> away, but it won't happen in the near future).

Done.

https://codereview.chromium.org/18023003/diff/1/sdk/lib/collection/hash_map.d...
sdk/lib/collection/hash_map.dart:33: iterable.forEach((e) {
On 2013/06/27 11:45:04, floitsch wrote:
> personally I prefer
> 
> for (var element in iterable) {
>   map[key(element)] = value(element);
> }
> 
> but that's just, because I expect it to be easier to get optimized.

Done.

https://codereview.chromium.org/18023003/diff/1/sdk/lib/collection/hash_map.d...
sdk/lib/collection/hash_map.dart:43: var keyIterator = keys.iterator;
On 2013/06/27 11:45:04, floitsch wrote:
> Use type.

Done.

https://codereview.chromium.org/18023003/diff/1/sdk/lib/collection/hash_map.d...
sdk/lib/collection/hash_map.dart:55: if (hasNextKey != hasNextValue) {
On 2013/06/27 11:45:04, floitsch wrote:
> if (hasNextKey || hasNextValue) {
> 
> It has the same effect, but, to me, it makes it more clear what happens.
> hasNextKey != hasNextValue could (in if implemented naively) be more
efficient,
> though.

Done.

https://codereview.chromium.org/18023003/diff/1/sdk/lib/core/map.dart
File sdk/lib/core/map.dart (right):

https://codereview.chromium.org/18023003/diff/1/sdk/lib/core/map.dart#newcode23
sdk/lib/core/map.dart:23: * Creates a map where each key is an element from
[keys] with
On 2013/06/27 11:45:04, floitsch wrote:
> Try a shorter first sentence. More details in the next paragraph.
> 
> Creates a [Map] associating the given [keys] to [values].
> 
> This constructor iterates over [keys] and [values] and maps each element of
> [keys] to the corresponding element of [values].
> 
> If [keys] contains the same object multiple times, the last occurrence
> overwrites the previous value.
> 
> It is an error ...

Done.

https://codereview.chromium.org/18023003/diff/1/sdk/lib/core/map.dart#newcode26
sdk/lib/core/map.dart:26: * The length of [keys] and [values] must be the same,
otherwise an ArgumentError
On 2013/06/27 11:45:04, floitsch wrote:
> long line.
> 
> We usually don't say what kind of error happens:
> 
> "It is an error if the two [Iterable]s don't have the same length".

Done.

https://codereview.chromium.org/18023003/diff/1/sdk/lib/core/map.dart#newcode33
sdk/lib/core/map.dart:33: * Creates a map where each key-value pair is computed
from each element in
On 2013/06/27 11:45:04, floitsch wrote:
> Creates a [Map] where the keys and values are computed from the [iterable].
> // new line to have a separate paragraph in the doc.
> For each element of the [iterable] this constructor computes a key/value pair,
> by applying [key] and [value] respectively.
> 
> The keys of the key/value pairs do not need to be unique. The last occurrence
of
> a key will simply overwrite any previous value.
> 
> If no values ...

Done.

https://codereview.chromium.org/18023003/diff/1/tests/corelib/map_from_iterab...
File tests/corelib/map_from_iterable_test.dart (right):

https://codereview.chromium.org/18023003/diff/1/tests/corelib/map_from_iterab...
tests/corelib/map_from_iterable_test.dart:52: print(map.length);
On 2013/06/27 11:45:04, floitsch wrote:
> debug print.

Done.

https://codereview.chromium.org/18023003/diff/1/tests/corelib/map_from_iterab...
tests/corelib/map_from_iterable_test.dart:103: }
On 2013/06/27 11:45:04, floitsch wrote:
> Add test that the returned map is of the right generic type.
> 
> var map = new Map<int, String>.fromIterable(...);
> Expect.isTrue(map is Map<int, String>);
> 
> // Make sure it is not just Map<dynamic, dynamic>.
> Expect.isFalse(map is Map<String, dynamic>);
> Expect.isFalse(map is Map<dynamic, int>);

Done.

https://codereview.chromium.org/18023003/diff/1/tests/corelib/map_from_iterab...
File tests/corelib/map_from_iterables_test.dart (right):

https://codereview.chromium.org/18023003/diff/1/tests/corelib/map_from_iterab...
tests/corelib/map_from_iterables_test.dart:17: var map = new
Map.fromIterables([1, 2, 3], ["one", "two", "three"]);
On 2013/06/27 11:45:04, floitsch wrote:
> write at least one test with generic types.

Done.

https://codereview.chromium.org/18023003/diff/1/tests/corelib/map_from_iterab...
tests/corelib/map_from_iterables_test.dart:48: void identicalElementsTest() {
On 2013/06/27 11:45:04, floitsch wrote:
> equalElementsTest (although they are identical here)

Done.

[floitsch, 2013-06-27 13:07:16]

still LGTM.

https://codereview.chromium.org/18023003/diff/10001/sdk/lib/core/map.dart
File sdk/lib/core/map.dart (right):

https://codereview.chromium.org/18023003/diff/10001/sdk/lib/core/map.dart#new...
sdk/lib/core/map.dart:39: * For each element of the [iterable] this constructor
computes a key/value pair,
long lines.

https://codereview.chromium.org/18023003/diff/10001/sdk/lib/core/map.dart#new...
sdk/lib/core/map.dart:42: * The keys of the key/value pairs do not need to be
unique. The last occurrence of
long line.

https://codereview.chromium.org/18023003/diff/10001/tests/corelib/map_from_it...
File tests/corelib/map_from_iterable_test.dart (right):

https://codereview.chromium.org/18023003/diff/10001/tests/corelib/map_from_it...
tests/corelib/map_from_iterable_test.dart:95: print(map.length);
remove print.

https://codereview.chromium.org/18023003/diff/10001/tests/corelib/map_from_it...
tests/corelib/map_from_iterable_test.dart:109: // Make sure it is not just
Map<dynamic, dynamic>.
indent comment too.

[zarah, 2013-06-27 13:13:13]

https://codereview.chromium.org/18023003/diff/10001/sdk/lib/core/map.dart
File sdk/lib/core/map.dart (right):

https://codereview.chromium.org/18023003/diff/10001/sdk/lib/core/map.dart#new...
sdk/lib/core/map.dart:39: * For each element of the [iterable] this constructor
computes a key/value pair,
On 2013/06/27 13:07:17, floitsch wrote:
> long lines.

Done.

https://codereview.chromium.org/18023003/diff/10001/sdk/lib/core/map.dart#new...
sdk/lib/core/map.dart:42: * The keys of the key/value pairs do not need to be
unique. The last occurrence of
On 2013/06/27 13:07:17, floitsch wrote:
> long line.

Done.

https://codereview.chromium.org/18023003/diff/10001/tests/corelib/map_from_it...
File tests/corelib/map_from_iterable_test.dart (right):

https://codereview.chromium.org/18023003/diff/10001/tests/corelib/map_from_it...
tests/corelib/map_from_iterable_test.dart:95: print(map.length);
On 2013/06/27 13:07:17, floitsch wrote:
> remove print.

Done.

https://codereview.chromium.org/18023003/diff/10001/tests/corelib/map_from_it...
tests/corelib/map_from_iterable_test.dart:109: // Make sure it is not just
Map<dynamic, dynamic>.
On 2013/06/27 13:07:17, floitsch wrote:
> indent comment too.

Done.

[zarah, 2013-06-27 13:17:25]

Committed patchset #4 manually as r24514 (presubmit successful).