Add Set.union, Set.difference. Change Set methods to expect Set.

Add Set.union, Set.difference. Change Set methods to expect Set.

Previously the Set methods expected a Collection, which made no
sense.

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

[floitsch, 2013-03-12 16:25:00]

LGTM (except iterable change).

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

https://codereview.chromium.org/12646005/diff/1/sdk/lib/collection/hash_set.d...
sdk/lib/collection/hash_set.dart:89: bool isSubsetOf(Set<E> other) {
bool isSubsetOf(Set<E> other) => other.containsAll(this);

And if there is a reason not to do this, explain.

https://codereview.chromium.org/12646005/diff/1/sdk/lib/collection/linked_has...
File sdk/lib/collection/linked_hash_set.dart (right):

https://codereview.chromium.org/12646005/diff/1/sdk/lib/collection/linked_has...
sdk/lib/collection/linked_hash_set.dart:131: bool isSubsetOf(Set<E> other) {
ditto. isSubsetOf(other) => other.containsAll(this);

https://codereview.chromium.org/12646005/diff/1/sdk/lib/collection/linked_has...
sdk/lib/collection/linked_hash_set.dart:131: bool isSubsetOf(Set<E> other) {
move all these methods to IterableMixinWorkaround?
For example IterableMixinWorkaround.isSubsetOfSet(set1, set2)

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

https://codereview.chromium.org/12646005/diff/1/sdk/lib/core/iterable.dart#ne...
sdk/lib/core/iterable.dart:21: * than once (for example once to get the length,
and then once to do something
Doesn't fit this CL.
Also I don't necessarily agree.

Maybe: Iterables may have side effects when iterating over them (for example as
a result of `map`). However, it is bad style to send them to other
(unsuspecting) functions. In general it should be reasonable to iterate over
iterables several times. For example, once to get the length, and once more to
do something for each element.

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

https://codereview.chromium.org/12646005/diff/1/sdk/lib/core/set.dart#newcode43
sdk/lib/core/set.dart:43: * Returns true if this Set contains all the elements
of
Fits on one line.

https://codereview.chromium.org/12646005/diff/1/sdk/lib/core/set.dart#newcode49
sdk/lib/core/set.dart:49: * Returns a new set which is the intersection between
this set and
ditto.

[Sean Eagan, 2013-03-12 18:43:55]

On 2013/03/12 11:29:25, Lasse Reichstein Nielsen wrote:

It seems like containsAll would be useful as a Iterable-based operation, defined
on Iterable, and receiving an Iterable argument.  Especially for Iterables which
are Set-like, but not actually Sets, such as Map.keys, and to be able to pass a
List literal:

map.keys.containsAll(['a', 'b']);

list.containsAll(iterable);
// vs.
iterable.every(list.contains);

...not much shorter, but a bit more readable.

And if the types are symmetric (Iterable expecting Iterable or Set expecting
Set) then why keep "isSubsetOf" since:

a.isSubsetOf(b)

is just:

b.containsAll(a);


Also, I was hoping "union", "intersection", and "difference" would be made lazy,
have you considered that ?

Here's how iteration could work on the returned wrapper Sets:

union:  iterate over the first Set, then iterate over second Set and only emit
the item if it is not contained in the first Set.

intersection: iterate over the first Set, and only emit the item if it is also
contained in the second Set.

difference: iterate over the first Set, and only include the item if it is not
contained in the second Set.

[Lasse Reichstein Nielsen, 2013-03-13 13:01:42]

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

https://codereview.chromium.org/12646005/diff/1/sdk/lib/collection/hash_set.d...
sdk/lib/collection/hash_set.dart:89: bool isSubsetOf(Set<E> other) {
The resason is the usual one: I don't want public, overridable methods to call
each other. I'll make a method in IterableMixinOverride (SetMixinOverride?) that
they can both use.

https://codereview.chromium.org/12646005/diff/1/sdk/lib/collection/linked_has...
File sdk/lib/collection/linked_hash_set.dart (right):

https://codereview.chromium.org/12646005/diff/1/sdk/lib/collection/linked_has...
sdk/lib/collection/linked_hash_set.dart:131: bool isSubsetOf(Set<E> other) {
On 2013/03/12 16:25:00, floitsch wrote:
> move all these methods to IterableMixinWorkaround?
> For example IterableMixinWorkaround.isSubsetOfSet(set1, set2)

Done.

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

https://codereview.chromium.org/12646005/diff/1/sdk/lib/core/iterable.dart#ne...
sdk/lib/core/iterable.dart:21: * than once (for example once to get the length,
and then once to do something
Removed for now.

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

https://codereview.chromium.org/12646005/diff/1/sdk/lib/core/set.dart#newcode43
sdk/lib/core/set.dart:43: * Returns true if this Set contains all the elements
of
On 2013/03/12 16:25:00, floitsch wrote:
> Fits on one line.

Done.

https://codereview.chromium.org/12646005/diff/1/sdk/lib/core/set.dart#newcode49
sdk/lib/core/set.dart:49: * Returns a new set which is the intersection between
this set and
On 2013/03/12 16:25:00, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/12646005/diff/1/sdk/lib/core/set.dart#newcode49
sdk/lib/core/set.dart:49: * Returns a new set which is the intersection between
this set and
On 2013/03/12 16:25:00, floitsch wrote:
> ditto.

Done.

[Lasse Reichstein Nielsen, 2013-03-13 13:05:58]

Committed patchset #1 manually as r19926 (presubmit successful).

[Lasse Reichstein Nielsen, 2013-03-14 07:18:17]

I don't think we will want lazy operations on Set that returns a Set.
If I were to add it, I'd make a LazySet class with
union/intersection/difference/symmetricDifference constructors, so you have to
be explicit about it.

The point about containsAll and isSubsetOf is good. We weren't looking at those
with an eye to change them, but we've decided to let containsAll allow any
Iterable. It's still only on Set since it really relies on a fast contains
method. At the same time we'll remove (starting by deprecating) the isSubsetOf
method.
That means that if you really want to do list.containsAll(otherList), you need
to convert the first list to a set first - which is what isSubsetOf currently
does anyway to avoid quadratic behavior.

So, thanks for the comments.