Wrote functions to convert collections and maps to strings and invoked

Wrote functions to convert collections and maps to strings and invoked
these functions from the toString methods of all built-in Collection
and Map implementations. Wrote smoke tests and basher tests.

BUG=4466785
Committed: https://code.google.com/p/dart/source/detail?r=4345

[jjb, 2012-02-02 17:30:06]

Tests not written yet. Also I am not 100% sure that I hit all implementations of
all interfaces.

[sra1, 2012-02-02 19:45:48]

On 2012/02/02 17:30:06, jjb wrote:
> Tests not written yet. Also I am not 100% sure that I hit all implementations
of
> all interfaces.

This corelib has some VM specific code (see byte_array.dart).

There is another - frog/lib/corelib.dart with Frog/JS specific code (and some
gratuitous differences.)  There is a lot of common #source-ing so P>.95 you have
broken that lib.

I am unclear on the status of compiler/lib/corelib.dart - it might still be
relevant to the first Dart-to-JS compiler 'dartc'.

(All three compilers (VM, Frog, Dartc) have different 'native' features.)

[sra1, 2012-02-02 19:46:01]

DBC

http://codereview.chromium.org/9320028/diff/1/corelib/src/implementation/hash...
File corelib/src/implementation/hash_map_set.dart (right):

http://codereview.chromium.org/9320028/diff/1/corelib/src/implementation/hash...
corelib/src/implementation/hash_map_set.dart:272: String toString() {
String toString() => Maps.mapToString(this);

Use the expression-form rather than block-form for functions and methods where
reasonable.  (This also works with void --> void)

If you are wondering about consistency, much of corelib predates the => syntax.
It might be a good, if tedious, task to convert the other functions, e.g.

void clear() => _backingMap.clear();

bool every(bool f(element)) => _backingMap.getKeys().every(f);

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

http://codereview.chromium.org/9320028/diff/1/runtime/lib/collections.dart#ne...
runtime/lib/collections.dart:73: * simply return the results of this method
applied to the collection.
So you can still get infinite regress by having naively defined user types?

class Cell {
   var value;
   Cell(this.value);
   toString() => 'Cell($value)';
}

var a = [];
a.add(new Cell(a));

http://codereview.chromium.org/9320028/diff/1/runtime/lib/collections.dart#ne...
runtime/lib/collections.dart:77: _emitCollection(c, result, new List());
'[]' is more Dart-idiomatic than 'new List()'

[jjb, 2012-02-02 20:04:55]

On 2012/02/02 19:45:48, sra1 wrote:
> On 2012/02/02 17:30:06, jjb wrote:
> > Tests not written yet. Also I am not 100% sure that I hit all
implementations
> of
> > all interfaces.
> 
> This corelib has some VM specific code (see byte_array.dart).
> 
> There is another - frog/lib/corelib.dart with Frog/JS specific code (and some
> gratuitous differences.)  There is a lot of common #source-ing so P>.95 you
have
> broken that lib.
> 
> I am unclear on the status of compiler/lib/corelib.dart - it might still be
> relevant to the first Dart-to-JS compiler 'dartc'.
> 
> (All three compilers (VM, Frog, Dartc) have different 'native' features.)

It isn't clear what I should do as part of this CL.

[sra1, 2012-02-02 20:09:58]

On 2012/02/02 20:04:55, jjb wrote:
> On 2012/02/02 19:45:48, sra1 wrote:
> > On 2012/02/02 17:30:06, jjb wrote:
> > > Tests not written yet. Also I am not 100% sure that I hit all
> implementations
> > of
> > > all interfaces.
> > 
> > This corelib has some VM specific code (see byte_array.dart).
> > 
> > There is another - frog/lib/corelib.dart with Frog/JS specific code (and
some
> > gratuitous differences.)  There is a lot of common #source-ing so P>.95 you
> have
> > broken that lib.
> > 
> > I am unclear on the status of compiler/lib/corelib.dart - it might still be
> > relevant to the first Dart-to-JS compiler 'dartc'.
> > 
> > (All three compilers (VM, Frog, Dartc) have different 'native' features.)
> 
> It isn't clear what I should do as part of this CL.

I'll forward you an internal link to a preliminary description of the test
infrastructure.
Basically, you should make matching changes to the other corelibs to prevent
breakage,
and update them to the same functionality either in this CL or one to follow
shortly.

[jjb, 2012-02-02 20:30:33]

Thanks for taking a look at this!

By the way, I believe that our style recommendation to use superfluous braces
when control constructs are used on single statements is inconsistent with our
emphasis on concision.  I really with I could say:

      if (!first)
        result.add(", ");

Rather than:

      if (!first) {
        result.add(", ");
      }

I understand that I *can* say:

      if (!first) result.add(", ");

but I'm not sure it's more readable.

http://codereview.chromium.org/9320028/diff/1/corelib/src/implementation/hash...
File corelib/src/implementation/hash_map_set.dart (right):

http://codereview.chromium.org/9320028/diff/1/corelib/src/implementation/hash...
corelib/src/implementation/hash_map_set.dart:272: String toString() {
On 2012/02/02 19:46:01, sra1 wrote:
> String toString() => Maps.mapToString(this);
> 
> Use the expression-form rather than block-form for functions and methods where
> reasonable.  (This also works with void --> void)
> 
> If you are wondering about consistency, much of corelib predates the =>
syntax.
> It might be a good, if tedious, task to convert the other functions, e.g.
> 
> void clear() => _backingMap.clear();
> 
> bool every(bool f(element)) => _backingMap.getKeys().every(f);
> 
> 
> 

I considered this, but opted for the long form to match the stye in the files I
was editing.  That said, I'm more than willing to change the entire files.

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

http://codereview.chromium.org/9320028/diff/1/runtime/lib/collections.dart#ne...
runtime/lib/collections.dart:73: * simply return the results of this method
applied to the collection.
On 2012/02/02 19:46:01, sra1 wrote:
> So you can still get infinite regress by having naively defined user types?
> 
> class Cell {
>    var value;
>    Cell(this.value);
>    toString() => 'Cell($value)';
> }
> 
> var a = [];
> a.add(new Cell(a));
> 
> 

Yes, and I considered documenting this, but opted not to.  Java's
"Arrays.deeptToString" has the same problem, and no one ever bitched (to my
knowledge).

There is a way around it (static thread locals, which are simply static
variables in Dart), but it's dangerous, and I elected not to implement it.

http://codereview.chromium.org/9320028/diff/1/runtime/lib/collections.dart#ne...
runtime/lib/collections.dart:77: _emitCollection(c, result, new List());
On 2012/02/02 19:46:01, sra1 wrote:
> '[]' is more Dart-idiomatic than 'new List()'

Thanks.  What can the programmer assume about the implementation (performace,
mutability, etc.)?  Where is it documented?

[sra1, 2012-02-02 20:57:30]

On 2012/02/02 20:30:33, jjb wrote:
> Thanks for taking a look at this!
> 
> By the way, I believe that our style recommendation to use superfluous braces
> when control constructs are used on single statements is inconsistent with our
> emphasis on concision.  I really with I could say:
> 
>       if (!first)
>         result.add(", ");
> 
> Rather than:
> 
>       if (!first) {
>         result.add(", ");
>       }
> 
> I understand that I *can* say:
> 
>       if (!first) result.add(", ");
> 
> but I'm not sure it's more readable.

I completely agree with you here.
I find single-line is less readable, especially when the condition is more
substantial than the action.
I would certainly +1 anything you said to the broader Google community (since
this is a Google style guide issue.)


> 
>
http://codereview.chromium.org/9320028/diff/1/corelib/src/implementation/hash...
> File corelib/src/implementation/hash_map_set.dart (right):
> 
>
http://codereview.chromium.org/9320028/diff/1/corelib/src/implementation/hash...
> corelib/src/implementation/hash_map_set.dart:272: String toString() {
> On 2012/02/02 19:46:01, sra1 wrote:
> > String toString() => Maps.mapToString(this);
> > 
> > Use the expression-form rather than block-form for functions and methods
where
> > reasonable.  (This also works with void --> void)
> > 
> > If you are wondering about consistency, much of corelib predates the =>
> syntax.
> > It might be a good, if tedious, task to convert the other functions, e.g.
> > 
> > void clear() => _backingMap.clear();
> > 
> > bool every(bool f(element)) => _backingMap.getKeys().every(f);
> > 
> > 
> > 
> 
> I considered this, but opted for the long form to match the stye in the files
I
> was editing.  That said, I'm more than willing to change the entire files.
> 
> http://codereview.chromium.org/9320028/diff/1/runtime/lib/collections.dart
> File runtime/lib/collections.dart (right):
> 
>
http://codereview.chromium.org/9320028/diff/1/runtime/lib/collections.dart#ne...
> runtime/lib/collections.dart:73: * simply return the results of this method
> applied to the collection.
> On 2012/02/02 19:46:01, sra1 wrote:
> > So you can still get infinite regress by having naively defined user types?
> > 
> > class Cell {
> >    var value;
> >    Cell(this.value);
> >    toString() => 'Cell($value)';
> > }
> > 
> > var a = [];
> > a.add(new Cell(a));
> > 
> > 
> 
> Yes, and I considered documenting this, but opted not to.  Java's
> "Arrays.deeptToString" has the same problem, and no one ever bitched (to my
> knowledge).
> 
> There is a way around it (static thread locals, which are simply static
> variables in Dart), but it's dangerous, and I elected not to implement it.
> 
>
http://codereview.chromium.org/9320028/diff/1/runtime/lib/collections.dart#ne...
> runtime/lib/collections.dart:77: _emitCollection(c, result, new List());
> On 2012/02/02 19:46:01, sra1 wrote:
> > '[]' is more Dart-idiomatic than 'new List()'
> 
> Thanks.  What can the programmer assume about the implementation (performace,
> mutability, etc.)?  Where is it documented?

[sra1, 2012-02-02 20:57:58]

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

http://codereview.chromium.org/9320028/diff/1/runtime/lib/collections.dart#ne...
runtime/lib/collections.dart:73: * simply return the results of this method
applied to the collection.
On 2012/02/02 20:30:33, jjb wrote:
> On 2012/02/02 19:46:01, sra1 wrote:
> > So you can still get infinite regress by having naively defined user types?
> > 
> > class Cell {
> >    var value;
> >    Cell(this.value);
> >    toString() => 'Cell($value)';
> > }
> > 
> > var a = [];
> > a.add(new Cell(a));
> > 
> > 
> 
> Yes, and I considered documenting this, but opted not to.  Java's
> "Arrays.deeptToString" has the same problem, and no one ever bitched (to my
> knowledge).
> 
> There is a way around it (static thread locals, which are simply static
> variables in Dart), but it's dangerous, and I elected not to implement it.

Thats fine.

http://codereview.chromium.org/9320028/diff/1/runtime/lib/collections.dart#ne...
runtime/lib/collections.dart:77: _emitCollection(c, result, new List());
On 2012/02/02 20:30:33, jjb wrote:
> On 2012/02/02 19:46:01, sra1 wrote:
> > '[]' is more Dart-idiomatic than 'new List()'
> 
> Thanks.  What can the programmer assume about the implementation (performace,
> mutability, etc.)?  Where is it documented?

The best we have is:

10.6
https://docs.google.com/a/google.com/document/d/1YblLwWryUj0VZBJ2xBgat4gYgH0m...

You should be able to add comments - if not, ask Gilad.

I have added a comment.

[Ivan Posva, 2012-02-02 23:39:35]

https://chromiumcodereview.appspot.com/9320028/diff/1/runtime/lib/collections...
File runtime/lib/collections.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/1/runtime/lib/collections...
runtime/lib/collections.dart:94: result.add(isList ? "[" : "{");
The one confusing part for me here is that both Maps and non-List collections
(e.g. Sets) print with "{".

[sra1, 2012-02-04 08:42:50]

https://chromiumcodereview.appspot.com/9320028/diff/1/runtime/lib/collections...
File runtime/lib/collections.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/1/runtime/lib/collections...
runtime/lib/collections.dart:94: result.add(isList ? "[" : "{");
On 2012/02/02 23:39:35, Ivan Posva wrote:
> The one confusing part for me here is that both Maps and non-List collections
> (e.g. Sets) print with "{".

I think a set should print as {1, 2, 3}.
you can tell the difference between a set and a map by whether the 'elements'
look like values or associations "k: v".
I'm more concerned about a non-list non-set collection like Queue.
I think we can use a prefix and reuse this code and make them print as, e.g.
Queue{1, 2, 3}

We do need to keep the code small.  Since it is reachable from toString it will
be almost impossible to tree-shake.

[Bob Nystrom, 2012-02-06 18:23:17]

Drive-by comments...

https://chromiumcodereview.appspot.com/9320028/diff/1/corelib/src/implementat...
File corelib/src/implementation/maps.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/1/corelib/src/implementat...
corelib/src/implementation/maps.dart:97: result.add("{");
Either way is OK, but we generally default to single-quoted stings unless the
string happens to contain an apostrophe.

https://chromiumcodereview.appspot.com/9320028/diff/1/runtime/lib/collections...
File runtime/lib/collections.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/1/runtime/lib/collections...
runtime/lib/collections.dart:10: class Collections {
I believe this class is older than top-level functions in Dart. Now that we have
those, what do you think of eliminating this and just making them functions?

[jjb, 2012-02-06 19:10:03]

Thanks for the review.

https://chromiumcodereview.appspot.com/9320028/diff/1/corelib/src/implementat...
File corelib/src/implementation/maps.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/1/corelib/src/implementat...
corelib/src/implementation/maps.dart:97: result.add("{");
On 2012/02/06 18:23:17, Bob Nystrom wrote:
> Either way is OK, but we generally default to single-quoted stings unless the
> string happens to contain an apostrophe.

Interesting!  This is the opposite of what I'm used to from many other "curly
braces languages." Given that we're trying to appeal to JavaScript and Java
programmers, have we made the right choice?  If so, should we make this clear in
your style guide (http://www.dartlang.org/articles/idiomatic-dart)?

https://chromiumcodereview.appspot.com/9320028/diff/1/runtime/lib/collections...
File runtime/lib/collections.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/1/runtime/lib/collections...
runtime/lib/collections.dart:10: class Collections {
On 2012/02/06 18:23:17, Bob Nystrom wrote:
> I believe this class is older than top-level functions in Dart. Now that we
have
> those, what do you think of eliminating this and just making them functions?

Probably, though we'd still have to figure out where they should reside. Of
course this should be done later, and all at once.  And we'll have to fix a fair
amount of preexisting client code.

[Bob Nystrom, 2012-02-06 20:37:12]

https://chromiumcodereview.appspot.com/9320028/diff/1/corelib/src/implementat...
File corelib/src/implementation/maps.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/1/corelib/src/implementat...
corelib/src/implementation/maps.dart:97: result.add("{");
On 2012/02/06 19:10:03, jjb wrote:
> On 2012/02/06 18:23:17, Bob Nystrom wrote:
> > Either way is OK, but we generally default to single-quoted stings unless
the
> > string happens to contain an apostrophe.
> 
> Interesting!  This is the opposite of what I'm used to from many other "curly
> braces languages." Given that we're trying to appeal to JavaScript and Java
> programmers, have we made the right choice?

I think so. We find that we very frequently have HTML inside string literals and
want to use double quotes for attributes. That's easy if you're in the habit of
using single quotes for your strings:  '<div class="blah">'.

That being said, I find it easy to trip up when using contractions: 'this isn't
right'.

>  If so, should we make this clear in
> your style guide (http://www.dartlang.org/articles/idiomatic-dart%29?

I've got a note on my todo-list to add that. It isn't a huge priority (because
it's a pretty slight preference for single-quoted strings) but if I get a
chance, I'll probably add it.

https://chromiumcodereview.appspot.com/9320028/diff/1/runtime/lib/collections...
File runtime/lib/collections.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/1/runtime/lib/collections...
runtime/lib/collections.dart:10: class Collections {
On 2012/02/06 19:10:03, jjb wrote:
> On 2012/02/06 18:23:17, Bob Nystrom wrote:
> > I believe this class is older than top-level functions in Dart. Now that we
> have
> > those, what do you think of eliminating this and just making them functions?
> 
> Probably, though we'd still have to figure out where they should reside. Of
> course this should be done later, and all at once.  And we'll have to fix a
fair
> amount of preexisting client code.

Understood. It wouldn't be a trivial change, but I thought it might be something
for you to keep in mind. Dart gives us a few more toys to play with when
designing libraries than Java does.

[jjb, 2012-02-15 02:45:39]

PTAL

I have written a reasonably thorough test. It does not yet cover a few
implementations (e.g., fixed length lists and immutable collections) but it will
be trivial to add those. Also I added code to give queues the proper string
representation.

I have yet to address some review comments from patch set 1, but I'll do this
before final submission.

Thanks,

Josh

[sra1, 2012-02-15 03:11:10]

https://chromiumcodereview.appspot.com/9320028/diff/10001/tests/corelib/src/C...
File tests/corelib/src/CollectionToStringTest.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/10001/tests/corelib/src/C...
tests/corelib/src/CollectionToStringTest.dart:15: exactTest();
The tests are large and require careful reading.
It would be nice to see a few simpler tests that give confidence that the
algorithmic tests are testing what we expect, e.g

Expect.equals('[]', [].toString);

Expect.equals('[1,two]', [1, 'two'].toString());
Expect.equals('[1,two]', <String>['1', 'two'].toString());

[jjb, 2012-02-15 04:21:54]

On 2012/02/15 03:11:10, sra1 wrote:
>
https://chromiumcodereview.appspot.com/9320028/diff/10001/tests/corelib/src/C...
> File tests/corelib/src/CollectionToStringTest.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9320028/diff/10001/tests/corelib/src/C...
> tests/corelib/src/CollectionToStringTest.dart:15: exactTest();
> The tests are large and require careful reading.
> It would be nice to see a few simpler tests that give confidence that the
> algorithmic tests are testing what we expect, e.g
> 
> Expect.equals('[]', [].toString);
> 
> Expect.equals('[1,two]', [1, 'two'].toString());
> Expect.equals('[1,two]', <String>['1', 'two'].toString());

Sure, I'd be happy to a few like that.  I can guarantee you that the first one
is getting tested, as it's the only zero-length growable array ;)

[Ivan Posva, 2012-02-15 09:54:10]

LGTM with the added smoke tests.

-Ivan

https://chromiumcodereview.appspot.com/9320028/diff/1/corelib/src/implementat...
File corelib/src/implementation/maps.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/1/corelib/src/implementat...
corelib/src/implementation/maps.dart:97: result.add("{");
On 2012/02/06 18:23:17, Bob Nystrom wrote:
> Either way is OK, but we generally default to single-quoted stings unless the
> string happens to contain an apostrophe.

I think the choice is really personal preference. As noted below there are
stumbling blocks with both defaults. The resolution of a project wide preference
should not block this change.

https://chromiumcodereview.appspot.com/9320028/diff/1/runtime/lib/collections...
File runtime/lib/collections.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/1/runtime/lib/collections...
runtime/lib/collections.dart:77: _emitCollection(c, result, new List());
On 2012/02/02 19:46:01, sra1 wrote:
> '[]' is more Dart-idiomatic than 'new List()'

Again that really depends on your personal preference. I am fine either way and
for the use here I do actually prefer the "new List()" even if some other people
would have their fingers fall off because of too much typing.

https://chromiumcodereview.appspot.com/9320028/diff/10001/runtime/lib/collect...
File runtime/lib/collections.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/10001/runtime/lib/collect...
runtime/lib/collections.dart:53: // todo(jjb): visiting list should be an
identityHashSet when it exists
All caps TODO.

https://chromiumcodereview.appspot.com/9320028/diff/10001/tests/corelib/src/C...
File tests/corelib/src/CollectionToStringTest.dart (right):

https://chromiumcodereview.appspot.com/9320028/diff/10001/tests/corelib/src/C...
tests/corelib/src/CollectionToStringTest.dart:9: // todo(jjb): seed random
number generator when API allows it
Allcaps TODO. Here and maybe other places.

https://chromiumcodereview.appspot.com/9320028/diff/10001/tests/corelib/src/C...
tests/corelib/src/CollectionToStringTest.dart:15: exactTest();
On 2012/02/15 03:11:10, sra1 wrote:
> The tests are large and require careful reading.
> It would be nice to see a few simpler tests that give confidence that the
> algorithmic tests are testing what we expect, e.g
> 
> Expect.equals('[]', [].toString);
> 
> Expect.equals('[1,two]', [1, 'two'].toString());
> Expect.equals('[1,two]', <String>['1', 'two'].toString());

Adding a couple of these before starting the more involved tests would really
help with smoke testing this. It has the additional benefit of checking the
literals, so you should also include a couple of "const [1, 2]" style literals.