Change Set.toSet to always return a set with the same behavior.

The toSet operation returns a new Set with the same equality and iteration behavior, and with the same elements.

R=floitsch@google.com, jmesserly@google.com, sgjesse@google.com

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

[floitsch, 2014-05-15 14:08:31]

Implementation LGTM, but why do we need this?
Isn't toSet() enough?
Do we start to add clone() to all collection classes? to Map?

[Lasse Reichstein Nielsen, 2014-05-19 07:30:05]

I changed clone to just using .toSet() and updated the toSet documentation to
promise that this works.

That makes cloneEmpty, and without clone around, the name is even worse. What
could it be renamed to?
- toEmptySet
- createEmpytSet
- newEmptySet
- newSet
? 
Suggestions appreciated!

https://codereview.chromium.org/288103003/diff/20001/pkg/third_party/html5lib...
File pkg/third_party/html5lib/lib/src/css_class_set.dart (right):

https://codereview.chromium.org/288103003/diff/20001/pkg/third_party/html5lib...
pkg/third_party/html5lib/lib/src/css_class_set.dart:282: Set<E> cloneEmpty() =>
new LinkedHashSet<String>();
@jmesserly:
What does it mean that this is in pkg/third_party?
I expected pkg to be the packages we are maintaining as part of the repository,
so what does third_party mean here? Is this maintained in another repository
somewhere?
(I.e., should I revert the change or should I update the pubspec version?)

[floitsch, 2014-05-19 13:08:14]

As discussed in person:
I don't think the Set should have this method. However, a SetBase or SetMixin
could require it.

This makes it hard for us to reuse the public implementation since we don't want
this method to be public on our implementation sets (although we could discuss
this, too.

On 2014/05/19 07:30:05, Lasse Reichstein Nielsen wrote:
> I changed clone to just using .toSet() and updated the toSet documentation to
> promise that this works.
> 
> That makes cloneEmpty, and without clone around, the name is even worse. What
> could it be renamed to?
> - toEmptySet
> - createEmpytSet
> - newEmptySet
> - newSet
> ? 
> Suggestions appreciated!
> 
>
https://codereview.chromium.org/288103003/diff/20001/pkg/third_party/html5lib...
> File pkg/third_party/html5lib/lib/src/css_class_set.dart (right):
> 
>
https://codereview.chromium.org/288103003/diff/20001/pkg/third_party/html5lib...
> pkg/third_party/html5lib/lib/src/css_class_set.dart:282: Set<E> cloneEmpty()
=>
> new LinkedHashSet<String>();
> @jmesserly:
> What does it mean that this is in pkg/third_party?
> I expected pkg to be the packages we are maintaining as part of the
repository,
> so what does third_party mean here? Is this maintained in another repository
> somewhere?
> (I.e., should I revert the change or should I update the pubspec version?)

[Jennifer Messerly, 2014-05-19 17:44:42]

dart/pkg html impl changes LGTM.

My random opinion, it's kinda nice to have the ability to create a new set of
the same type (though I have no idea what to name the method ... ".newSet()" or
".createSet()" reads the best I think). If you want to make a wrapper set, being
able to create one of the same type is good... ObservableMap works roughly that
way:
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/pkg/...

Right now it only supports the Map types I explicitly listed.

Then again, I'm not super happy with ObservableMap anyway (e.g. being a wrapper
adds memory overhead/indirection), so maybe not the best example.

https://codereview.chromium.org/288103003/diff/20001/pkg/third_party/html5lib...
File pkg/third_party/html5lib/lib/src/css_class_set.dart (right):

https://codereview.chromium.org/288103003/diff/20001/pkg/third_party/html5lib...
pkg/third_party/html5lib/lib/src/css_class_set.dart:282: Set<E> cloneEmpty() =>
new LinkedHashSet<String>();
On 2014/05/19 07:30:05, Lasse Reichstein Nielsen wrote:
> @jmesserly:
> What does it mean that this is in pkg/third_party?
> I expected pkg to be the packages we are maintaining as part of the
repository,
> so what does third_party mean here? Is this maintained in another repository
> somewhere?
> (I.e., should I revert the change or should I update the pubspec version?)
> 

it means it's under a different license (MIT) and has different authors (listed
in pkg/third_party/html5lib/LICENSE).

IIRC, third_party was advice from the lawyers way back. we tried
"pkg/third_party" vs "third_party/pkg" but it's still confusing. kevmoo was
going to follow up and see if we can do something different. Sorry about the
confusion ... confuses me all the time too :-)

Not sure if we were tracking it, so I opened a bug for Kevin:
https://code.google.com/p/dart/issues/detail?id=18883

[Lasse Reichstein Nielsen, 2014-05-21 12:55:16]

I've remove cloneEmpty for now. I'll see if I can make a useful SetMixin without
it.

[Søren Gjesse, 2014-05-22 07:45:01]

lgtm

[Lasse Reichstein Nielsen, 2014-05-22 08:30:12]

Committed patchset #5 manually as r36473 (presubmit successful).

[Siggi Cherem (dart-lang), 2014-05-23 21:59:31]

https://codereview.chromium.org/288103003/diff/80001/pkg/third_party/html5lib...
File pkg/third_party/html5lib/pubspec.yaml (right):

https://codereview.chromium.org/288103003/diff/80001/pkg/third_party/html5lib...
pkg/third_party/html5lib/pubspec.yaml:2: version: 0.12.0-dev
Just so you know so you are not surprised, I changed this version back.
Basically, after removing the cloneEmpty method from this CL, there were no
changes in html5lib :)

[Lasse Reichstein Nielsen, 2014-05-23 22:09:27]

https://codereview.chromium.org/288103003/diff/80001/pkg/third_party/html5lib...
File pkg/third_party/html5lib/pubspec.yaml (right):

https://codereview.chromium.org/288103003/diff/80001/pkg/third_party/html5lib...
pkg/third_party/html5lib/pubspec.yaml:2: version: 0.12.0-dev
Ack, yes, my mistake.
I forgot I changed the version number.