Move toString() to collection classes.

Move toString() to collection classes.

R=floitsch@google.com

Reverted 24837

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

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

[floitsch, 2013-07-08 12:00:50]

FYI.

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

https://codereview.chromium.org/18837002/diff/1/runtime/lib/array.dart#newcode10
runtime/lib/array.dart:10: static List _toStringList = new List();
Add comment what this does.

https://codereview.chromium.org/18837002/diff/1/runtime/lib/array.dart#newcode19
runtime/lib/array.dart:19: for(int i = 0; i < _toStringList.length; i++) {
Change the call to `ToString.iterableToString` to
IterableMixinWorkaround.toStringList(), but otherwise keep the function.

Let the IterableMixinWorkaround deal with recursive lists.

https://codereview.chromium.org/18837002/diff/1/runtime/lib/array.dart#newcode20
runtime/lib/array.dart:20: if(identical(_toStringList[i], this))
Multiline `if`s always have {} for the body.
You can put it one one line, though.

https://codereview.chromium.org/18837002/diff/1/runtime/lib/array.dart#newcode25
runtime/lib/array.dart:25: _toStringList.remove(this);
You need to guard the removal with a `finally`. (Also add test for this case).
You must be the last one, otherwise something went wrong.
assert it, and then simply `removeLast`.

https://codereview.chromium.org/18837002/diff/1/runtime/lib/array.dart#newcod...
runtime/lib/array.dart:469: String toString() {
As above. Change to IterableMixinWorkaround.toStringList(this);

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

https://codereview.chromium.org/18837002/diff/1/runtime/lib/growable_array.da...
runtime/lib/growable_array.dart:341: for(int i = 0; i < _toStringList.length;
i++) {
change to IterableMixinWorkaround.toStringList(this);

https://codereview.chromium.org/18837002/diff/1/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/18837002/diff/1/runtime/lib/typed_data.dart#n...
runtime/lib/typed_data.dart:509: String toString() {
change to IterableMixinWorkaround.toStringList(this);

https://codereview.chromium.org/18837002/diff/1/sdk/lib/_collection_dev/itera...
File sdk/lib/_collection_dev/iterable.dart (right):

https://codereview.chromium.org/18837002/diff/1/sdk/lib/_collection_dev/itera...
sdk/lib/_collection_dev/iterable.dart:899: var result = new StringBuffer();
Move checking of recursive iterables in here.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/_collection_dev/itera...
sdk/lib/_collection_dev/iterable.dart:915: static String toStringList(List list)
{
Move checking of recursive lists in here.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/_internal/lib/js_arra...
File sdk/lib/_internal/lib/js_array.dart (right):

https://codereview.chromium.org/18837002/diff/1/sdk/lib/_internal/lib/js_arra...
sdk/lib/_internal/lib/js_array.dart:267: String toString() {
change to IterableMixinWorkaround.toStringList(this);

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

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/hash_set.d...
sdk/lib/collection/hash_set.dart:65: _toStringList.remove(this);
Same comments as before.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/hash_set.d...
sdk/lib/collection/hash_set.dart:66: print(result);
Remove debug print.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/linked_lis...
File sdk/lib/collection/linked_list.dart (right):

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/linked_lis...
sdk/lib/collection/linked_list.dart:66: for(int i = 0; i < _toStringList.length;
i++) {
Simple call to IterableMixinWorkaround.
Add TODO that this should be removed, since it should be inherited by
IterableMixin.

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

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/list.dart#...
sdk/lib/collection/list.dart:482: String toString() {
The ListMixin does not redirect to the Workaround anymore. It should have
working code.

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

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/maps.dart#...
sdk/lib/collection/maps.dart:63: static List _toStringList = new List();
add comment.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/maps.dart#...
sdk/lib/collection/maps.dart:82: for(int i = 0; i < _toStringList.length; i++) {
Same comments as for the other toString methods. (most importantly the missing
try/finally).

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

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/queue.dart...
sdk/lib/collection/queue.dart:307: for(int i = 0; i < _toStringList.length; i++)
{
Redirect to IterableMixinWorkaround for now. Add TODO, that code should be
inherited by IterableBase.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/queue.dart...
sdk/lib/collection/queue.dart:543: String toString() {
ditto.

[zarah, 2013-07-08 14:35:15]

PTAL

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

https://codereview.chromium.org/18837002/diff/1/runtime/lib/array.dart#newcode10
runtime/lib/array.dart:10: static List _toStringList = new List();
On 2013/07/08 12:00:50, floitsch wrote:
> Add comment what this does.

Done.

https://codereview.chromium.org/18837002/diff/1/runtime/lib/array.dart#newcode19
runtime/lib/array.dart:19: for(int i = 0; i < _toStringList.length; i++) {
On 2013/07/08 12:00:50, floitsch wrote:
> Change the call to `ToString.iterableToString` to
> IterableMixinWorkaround.toStringList(), but otherwise keep the function.
> 
> Let the IterableMixinWorkaround deal with recursive lists.

Done.

https://codereview.chromium.org/18837002/diff/1/runtime/lib/array.dart#newcode20
runtime/lib/array.dart:20: if(identical(_toStringList[i], this))
On 2013/07/08 12:00:50, floitsch wrote:
> Multiline `if`s always have {} for the body.
> You can put it one one line, though.

Done.

https://codereview.chromium.org/18837002/diff/1/runtime/lib/array.dart#newcode25
runtime/lib/array.dart:25: _toStringList.remove(this);
On 2013/07/08 12:00:50, floitsch wrote:
> You need to guard the removal with a `finally`. (Also add test for this case).
> You must be the last one, otherwise something went wrong.
> assert it, and then simply `removeLast`.

Done.

https://codereview.chromium.org/18837002/diff/1/runtime/lib/array.dart#newcod...
runtime/lib/array.dart:469: String toString() {
On 2013/07/08 12:00:50, floitsch wrote:
> As above. Change to IterableMixinWorkaround.toStringList(this);

Done.

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

https://codereview.chromium.org/18837002/diff/1/runtime/lib/growable_array.da...
runtime/lib/growable_array.dart:341: for(int i = 0; i < _toStringList.length;
i++) {
On 2013/07/08 12:00:50, floitsch wrote:
> change to IterableMixinWorkaround.toStringList(this);

Done.

https://codereview.chromium.org/18837002/diff/1/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/18837002/diff/1/runtime/lib/typed_data.dart#n...
runtime/lib/typed_data.dart:509: String toString() {
On 2013/07/08 12:00:50, floitsch wrote:
> change to IterableMixinWorkaround.toStringList(this);

Done.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/_collection_dev/itera...
File sdk/lib/_collection_dev/iterable.dart (right):

https://codereview.chromium.org/18837002/diff/1/sdk/lib/_collection_dev/itera...
sdk/lib/_collection_dev/iterable.dart:899: var result = new StringBuffer();
On 2013/07/08 12:00:50, floitsch wrote:
> Move checking of recursive iterables in here.

Done.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/_collection_dev/itera...
sdk/lib/_collection_dev/iterable.dart:915: static String toStringList(List list)
{
On 2013/07/08 12:00:50, floitsch wrote:
> Move checking of recursive lists in here.

Done.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/_internal/lib/js_arra...
File sdk/lib/_internal/lib/js_array.dart (right):

https://codereview.chromium.org/18837002/diff/1/sdk/lib/_internal/lib/js_arra...
sdk/lib/_internal/lib/js_array.dart:267: String toString() {
On 2013/07/08 12:00:50, floitsch wrote:
> change to IterableMixinWorkaround.toStringList(this);

Done.

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

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/hash_set.d...
sdk/lib/collection/hash_set.dart:65: _toStringList.remove(this);
On 2013/07/08 12:00:50, floitsch wrote:
> Same comments as before.

Done.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/hash_set.d...
sdk/lib/collection/hash_set.dart:66: print(result);
On 2013/07/08 12:00:50, floitsch wrote:
> Remove debug print.

Done.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/linked_lis...
File sdk/lib/collection/linked_list.dart (right):

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/linked_lis...
sdk/lib/collection/linked_list.dart:66: for(int i = 0; i < _toStringList.length;
i++) {
On 2013/07/08 12:00:50, floitsch wrote:
> Simple call to IterableMixinWorkaround.
> Add TODO that this should be removed, since it should be inherited by
> IterableMixin.

Done.

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

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/list.dart#...
sdk/lib/collection/list.dart:482: String toString() {
On 2013/07/08 12:00:50, floitsch wrote:
> The ListMixin does not redirect to the Workaround anymore. It should have
> working code.

Done.

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

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/maps.dart#...
sdk/lib/collection/maps.dart:63: static List _toStringList = new List();
On 2013/07/08 12:00:50, floitsch wrote:
> add comment.

Done.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/maps.dart#...
sdk/lib/collection/maps.dart:82: for(int i = 0; i < _toStringList.length; i++) {
On 2013/07/08 12:00:50, floitsch wrote:
> Same comments as for the other toString methods. (most importantly the missing
> try/finally).

Done.

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

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/queue.dart...
sdk/lib/collection/queue.dart:307: for(int i = 0; i < _toStringList.length; i++)
{
On 2013/07/08 12:00:50, floitsch wrote:
> Redirect to IterableMixinWorkaround for now. Add TODO, that code should be
> inherited by IterableBase.

Done.

https://codereview.chromium.org/18837002/diff/1/sdk/lib/collection/queue.dart...
sdk/lib/collection/queue.dart:543: String toString() {
On 2013/07/08 12:00:50, floitsch wrote:
> ditto.

Done.

[floitsch, 2013-07-08 16:12:34]

Some nits, but LGTM.

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/array.dart
File runtime/lib/array.dart (right):

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/array.dart#ne...
runtime/lib/array.dart:16: String toString() =>
IterableMixinWorkaround.toStringList(this);
VM devs prefer not to use "=>".

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/array.dart#ne...
runtime/lib/array.dart:457: String toString() =>
IterableMixinWorkaround.toStringList(this);
ditto.

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/growable_arra...
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/growable_arra...
runtime/lib/growable_array.dart:339: String toString() =>
IterableMixinWorkaround.toStringList(this);
VM devs prefer not to use "=>".

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/typed_data.da...
runtime/lib/typed_data.dart:508: String toString() =>
IterableMixinWorkaround.toStringList(this);
VM devs prefer not to use "=>".

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/_collection_dev/i...
File sdk/lib/_collection_dev/iterable.dart (right):

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:906: var result = new StringBuffer();
Type variable.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:911: for (var e in iterable) {
You could see, if iterable.join(", ") doesn't cost too much.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:919: } finally {
assert that you are the last.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:934: for (int i = 0; i < list.length; i++)
{
ditto. If list.join(", ") isn't too expensive it would remove code duplication.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/hash_s...
File sdk/lib/collection/hash_set.dart (right):

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/hash_s...
sdk/lib/collection/hash_set.dart:57: String toString() =>
IterableMixinWorkaround.toStringIterable(this);
Add TODO too.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/linked...
File sdk/lib/collection/linked_list.dart (right):

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/linked...
sdk/lib/collection/linked_list.dart:64: //TODO Remove this, and let it be
inherited by IterableMixin
Add space after "//".
TODOS are always of the form:
TODO(ldap): ..., or
TODO(issue-number): ...

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/list.dart
File sdk/lib/collection/list.dart (right):

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/list.d...
sdk/lib/collection/list.dart:492: for (int i = 0; i < length; i++) {
ditto. It would be nice if join wasn't too expensive.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/list.d...
sdk/lib/collection/list.dart:500: _toStringList.removeLast();
assert.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/maps.dart
File sdk/lib/collection/maps.dart (right):

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/maps.d...
sdk/lib/collection/maps.dart:103: _toStringList.remove(m);
removeLast (with assert).

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/queue....
File sdk/lib/collection/queue.dart (right):

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/queue....
sdk/lib/collection/queue.dart:304: //TODO Remove this, and let it be inherited
by IterableBase
ditto.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/queue....
sdk/lib/collection/queue.dart:532: //TODO Remove this, and let it be inherited
by IterableBase
ditto.

[zarah, 2013-07-08 16:57:46]

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/array.dart
File runtime/lib/array.dart (right):

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/array.dart#ne...
runtime/lib/array.dart:16: String toString() =>
IterableMixinWorkaround.toStringList(this);
On 2013/07/08 16:12:34, floitsch wrote:
> VM devs prefer not to use "=>".

Done.

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/array.dart#ne...
runtime/lib/array.dart:457: String toString() =>
IterableMixinWorkaround.toStringList(this);
On 2013/07/08 16:12:34, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/growable_arra...
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/growable_arra...
runtime/lib/growable_array.dart:339: String toString() =>
IterableMixinWorkaround.toStringList(this);
On 2013/07/08 16:12:34, floitsch wrote:
> VM devs prefer not to use "=>".

Done.

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/18837002/diff/14001/runtime/lib/typed_data.da...
runtime/lib/typed_data.dart:508: String toString() =>
IterableMixinWorkaround.toStringList(this);
On 2013/07/08 16:12:34, floitsch wrote:
> VM devs prefer not to use "=>".

Done.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/_collection_dev/i...
File sdk/lib/_collection_dev/iterable.dart (right):

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:906: var result = new StringBuffer();
On 2013/07/08 16:12:34, floitsch wrote:
> Type variable.

Done.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:911: for (var e in iterable) {
On 2013/07/08 16:12:34, floitsch wrote:
> You could see, if iterable.join(", ") doesn't cost too much.


Use writeAll instead since it is cheaper.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:919: } finally {
On 2013/07/08 16:12:34, floitsch wrote:
> assert that you are the last.

Done.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:934: for (int i = 0; i < list.length; i++)
{
On 2013/07/08 16:12:34, floitsch wrote:
> ditto. If list.join(", ") isn't too expensive it would remove code
duplication.

Use writeAll instead since it is cheaper.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/hash_s...
File sdk/lib/collection/hash_set.dart (right):

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/hash_s...
sdk/lib/collection/hash_set.dart:57: String toString() =>
IterableMixinWorkaround.toStringIterable(this);
On 2013/07/08 16:12:34, floitsch wrote:
> Add TODO too.

Done.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/linked...
File sdk/lib/collection/linked_list.dart (right):

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/linked...
sdk/lib/collection/linked_list.dart:64: //TODO Remove this, and let it be
inherited by IterableMixin
On 2013/07/08 16:12:34, floitsch wrote:
> Add space after "//".
> TODOS are always of the form:
> TODO(ldap): ..., or
> TODO(issue-number): ...

Done.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/list.dart
File sdk/lib/collection/list.dart (right):

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/list.d...
sdk/lib/collection/list.dart:492: for (int i = 0; i < length; i++) {
On 2013/07/08 16:12:34, floitsch wrote:
> ditto. It would be nice if join wasn't too expensive.

Use writeAll instead since it is cheaper.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/list.d...
sdk/lib/collection/list.dart:500: _toStringList.removeLast();
On 2013/07/08 16:12:34, floitsch wrote:
> assert.

Done.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/maps.dart
File sdk/lib/collection/maps.dart (right):

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/maps.d...
sdk/lib/collection/maps.dart:103: _toStringList.remove(m);
On 2013/07/08 16:12:34, floitsch wrote:
> removeLast (with assert).

Done.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/queue....
File sdk/lib/collection/queue.dart (right):

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/queue....
sdk/lib/collection/queue.dart:304: //TODO Remove this, and let it be inherited
by IterableBase
On 2013/07/08 16:12:34, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/18837002/diff/14001/sdk/lib/collection/queue....
sdk/lib/collection/queue.dart:532: //TODO Remove this, and let it be inherited
by IterableBase
On 2013/07/08 16:12:34, floitsch wrote:
> ditto.

Done.

[Lasse Reichstein Nielsen, 2013-07-09 06:15:44]

Drive-by pedantry.

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/array.dart
File runtime/lib/array.dart (right):

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/array.dart#ne...
runtime/lib/array.dart:9: 
You have added extra spaces here. Please remove them.
If possible, configure editor to delete spaces at end of line.

We generally don't want any invisible space, it makes it too easy to have a
change without knowing it, and make somebody else need to manually merge because
of it.

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/array.dart#ne...
runtime/lib/array.dart:19: 
Extras spaces (code-review tool is cool for detecting this :)

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/growable_arra...
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/growable_arra...
runtime/lib/growable_array.dart:342: 
Space.

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (left):

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/typed_data.da...
runtime/lib/typed_data.dart:511: 
Why remove line?

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/_collection_dev/i...
File sdk/lib/_collection_dev/iterable.dart (right):

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:915: 
Empty line added. Seems inconsistent. Consider removing it.

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:937: }
These two methods look almost, but not entirely, completely the same.
Would it be worth having a third method that takes delimiters as arguments as
well, and call it with either "{" amd "}", or with "[" and "]" for lists.

Alternatively, consider iterating the list using an index and see if you save
anything by not creating the iterator in writeAll.

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_array.dart (right):

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_array.dart:266: 
Extras spaces added.

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/collection/maps.dart
File sdk/lib/collection/maps.dart (right):

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/collection/maps.d...
sdk/lib/collection/maps.dart:62: 
Extra spaces added.

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/collection/maps.d...
sdk/lib/collection/maps.dart:96: first = false;
Consider moving this assignment into the condition.

I can't guarantee it will be faster, though.

https://codereview.chromium.org/18837002/diff/18003/tests/corelib/list_to_str...
File tests/corelib/list_to_string2_test.dart (right):

https://codereview.chromium.org/18837002/diff/18003/tests/corelib/list_to_str...
tests/corelib/list_to_string2_test.dart:19: 
Add comment:
  // Throwing in the middle of a toString does not leave the 
  // list as being visited.

https://codereview.chromium.org/18837002/diff/18003/tests/corelib/map_to_stri...
File tests/corelib/map_to_string_test.dart (right):

https://codereview.chromium.org/18837002/diff/18003/tests/corelib/map_to_stri...
tests/corelib/map_to_string_test.dart:17: Expect.throws(m.toString, (e) => e ==
"Bad!");
Try using ThrowOnToString as key as well.

[zarah, 2013-07-09 08:20:54]

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/array.dart
File runtime/lib/array.dart (right):

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/array.dart#ne...
runtime/lib/array.dart:9: 
On 2013/07/09 06:15:44, Lasse Reichstein Nielsen wrote:
> You have added extra spaces here. Please remove them.
> If possible, configure editor to delete spaces at end of line.
> 
> We generally don't want any invisible space, it makes it too easy to have a
> change without knowing it, and make somebody else need to manually merge
because
> of it.

Done.

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/array.dart#ne...
runtime/lib/array.dart:19: 
On 2013/07/09 06:15:44, Lasse Reichstein Nielsen wrote:
> Extras spaces (code-review tool is cool for detecting this :)

Done.

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/growable_arra...
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/growable_arra...
runtime/lib/growable_array.dart:342: 
On 2013/07/09 06:15:44, Lasse Reichstein Nielsen wrote:
> Space.

Done.

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (left):

https://codereview.chromium.org/18837002/diff/18003/runtime/lib/typed_data.da...
runtime/lib/typed_data.dart:511: 
On 2013/07/09 06:15:44, Lasse Reichstein Nielsen wrote:
> Why remove line?

Added back.

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/_collection_dev/i...
File sdk/lib/_collection_dev/iterable.dart (right):

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:915: 
On 2013/07/09 06:15:44, Lasse Reichstein Nielsen wrote:
> Empty line added. Seems inconsistent. Consider removing it.

Done.

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/_collection_dev/i...
sdk/lib/_collection_dev/iterable.dart:937: }
On 2013/07/09 06:15:44, Lasse Reichstein Nielsen wrote:
> These two methods look almost, but not entirely, completely the same.
> Would it be worth having a third method that takes delimiters as arguments as
> well, and call it with either "{" amd "}", or with "[" and "]" for lists.
> 
> Alternatively, consider iterating the list using an index and see if you save
> anything by not creating the iterator in writeAll.

Done.

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_array.dart (right):

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_array.dart:266: 
On 2013/07/09 06:15:44, Lasse Reichstein Nielsen wrote:
> Extras spaces added.

Done.

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/collection/maps.dart
File sdk/lib/collection/maps.dart (right):

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/collection/maps.d...
sdk/lib/collection/maps.dart:62: 
On 2013/07/09 06:15:44, Lasse Reichstein Nielsen wrote:
> Extra spaces added.

Done.

https://codereview.chromium.org/18837002/diff/18003/sdk/lib/collection/maps.d...
sdk/lib/collection/maps.dart:96: first = false;
On 2013/07/09 06:15:44, Lasse Reichstein Nielsen wrote:
> Consider moving this assignment into the condition.
> 
> I can't guarantee it will be faster, though.

Not done, as discussed.

https://codereview.chromium.org/18837002/diff/18003/tests/corelib/list_to_str...
File tests/corelib/list_to_string2_test.dart (right):

https://codereview.chromium.org/18837002/diff/18003/tests/corelib/list_to_str...
tests/corelib/list_to_string2_test.dart:19: 
On 2013/07/09 06:15:44, Lasse Reichstein Nielsen wrote:
> Add comment:
>   // Throwing in the middle of a toString does not leave the 
>   // list as being visited.

Done.

https://codereview.chromium.org/18837002/diff/18003/tests/corelib/map_to_stri...
File tests/corelib/map_to_string_test.dart (right):

https://codereview.chromium.org/18837002/diff/18003/tests/corelib/map_to_stri...
tests/corelib/map_to_string_test.dart:17: Expect.throws(m.toString, (e) => e ==
"Bad!");
On 2013/07/09 06:15:44, Lasse Reichstein Nielsen wrote:
> Try using ThrowOnToString as key as well.

Done.

[zarah, 2013-07-09 08:36:05]

Committed patchset #6 manually as r24836 (presubmit successful).

[zarah, 2013-07-11 12:59:23]

TBR

[zarah, 2013-07-11 12:59:24]

Committed patchset #7 manually as r24913 (presubmit successful).