Add methods to Collection.

Add methods to Collection.

Methods are:
- void add(E)
- void addAll(Iterable<E>)
- void remove(Object)
- void removeAll(Iterable)
- void retainAll(Iterable)
- void removeMatching(bool test(E))
- void retainMatching(bool test(E))

They have been implemented for List and Set only.

Default implementations of removeX methods in Collections assume an efficient
remove method, which Lists won't have.

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

[floitsch, 2013-01-17 13:36:58]

LGTM.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/collection/collect...
File sdk/lib/collection/collections.dart (right):

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/collection/collect...
sdk/lib/collection/collections.dart:53: * is efficient, which it isn't for,
e.g., [List]s.
too much "." and "," and latin.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.dart
File sdk/lib/core/collection.dart (right):

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:28: int get length;
I would remove this line.

For example: should a Scheme-like linked list not be a Collection?

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:33: * After this, the [contains] method should
return true for that
I would remove this line. Seems obvious.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:44: void addAll(Iterable<E> elements);
Provide default implementation.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:49: * This removes only one instance of the element
for collections that can
If the collection contains the element more than once, only one occurrence is
removed. The collection decides which one it removes.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:63: void removeAll(Iterable elements);
Provide default implementation.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:69: * Afterwards this collection should contain
only elements that
Seems obvious.

Maybe mention "intersection" ?

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:72: void retainAll(Iterable elements);
Provide default implementation.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:79: void removeMatching(bool test(E element));
Provide default implementation.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:84: * An elements [:e:] satisfies [test] if
[:test(e):] is true.
... [:e:] fails to satisfy [test] if [:test(e):] is not equal to true.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:86: void retainMatching(bool test(E element));
Provide default implementation.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/list.dart
File sdk/lib/core/list.dart (right):

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:309: "Cannot remove in an unmodifiable list");
remove from ...

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/queue.dart
File sdk/lib/core/queue.dart (right):

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/queue.dart#ne...
sdk/lib/core/queue.dart:215: void removeAll(Iterable elements) {
If Collection provides default implementations, this method should not be
necessary.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/queue.dart#ne...
sdk/lib/core/queue.dart:225: void retainAll(Iterable elements) {
ditto.

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

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/set.dart#newc...
sdk/lib/core/set.dart:35: bool remove(Object value);
make it "var" ?

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/set.dart#newc...
sdk/lib/core/set.dart:77: class _HashSetImpl<E> extends Iterable<E> implements
HashSet<E> {
extends Collection
or even
extends HashSet<E>
?

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/set.dart#newc...
sdk/lib/core/set.dart:109: void removeAll(Iterable elements) {
should not be necessary if Collection provides default implementations.
Same below.

https://codereview.chromium.org/11931034/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/11931034/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:93: void remove(Object
object) {
Why doesn't this class need the other methods? Is it, because the listMixin is
applied to this class?

[Lasse Reichstein Nielsen, 2013-01-18 11:41:48]

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/collection/collect...
File sdk/lib/collection/collections.dart (right):

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/collection/collect...
sdk/lib/collection/collections.dart:53: * is efficient, which it isn't for,
e.g., [List]s.
On 2013/01/17 13:36:58, floitsch wrote:
> too much "." and "," and latin.

Done.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.dart
File sdk/lib/core/collection.dart (right):

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:28: int get length;
Reasonable. We can write on List and Set that they are efficient.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:33: * After this, the [contains] method should
return true for that
On 2013/01/17 13:36:58, floitsch wrote:
> I would remove this line. Seems obvious.

Done.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:44: void addAll(Iterable<E> elements);
On 2013/01/17 13:36:58, floitsch wrote:
> Provide default implementation.

Done.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:63: void removeAll(Iterable elements);
On 2013/01/17 13:36:58, floitsch wrote:
> Provide default implementation.

Done.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:69: * Afterwards this collection should contain
only elements that
On 2013/01/17 13:36:58, floitsch wrote:
> Seems obvious.
> 
> Maybe mention "intersection" ?

Done.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:72: void retainAll(Iterable elements);
On 2013/01/17 13:36:58, floitsch wrote:
> Provide default implementation.

Done.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:79: void removeMatching(bool test(E element));
On 2013/01/17 13:36:58, floitsch wrote:
> Provide default implementation.

Done.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:84: * An elements [:e:] satisfies [test] if
[:test(e):] is true.
Double negative is not an improvement, and it says the same thing.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/collection.da...
sdk/lib/core/collection.dart:86: void retainMatching(bool test(E element));
On 2013/01/17 13:36:58, floitsch wrote:
> Provide default implementation.

Done.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/list.dart
File sdk/lib/core/list.dart (right):

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:309: "Cannot remove in an unmodifiable list");
On 2013/01/17 13:36:58, floitsch wrote:
> remove from ...

Done.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/queue.dart
File sdk/lib/core/queue.dart (right):

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/queue.dart#ne...
sdk/lib/core/queue.dart:215: void removeAll(Iterable elements) {
This implementation is not the default implementation.
The default implementation of Collections.removeAll assumes efficient remove, so
successive calls to remove is efficient.
This implementation instead creates an efficient check for being in elements and
uses removeMatching.

I'll add this as Collections.removeAllList.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/queue.dart#ne...
sdk/lib/core/queue.dart:225: void retainAll(Iterable elements) {
This one is the same as the default, so it can be removed.

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

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/set.dart#newc...
sdk/lib/core/set.dart:35: bool remove(Object value);
Object is correct here. We only use ==, possibly hashCode.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/set.dart#newc...
sdk/lib/core/set.dart:77: class _HashSetImpl<E> extends Iterable<E> implements
HashSet<E> {
Must be Collection, HashSet doesn't have a generative constructor.

Pah, I'll just remove _HashSetImpl and make HashSet be it.

https://codereview.chromium.org/11931034/diff/2001/sdk/lib/core/set.dart#newc...
sdk/lib/core/set.dart:109: void removeAll(Iterable elements) {
On 2013/01/17 13:36:58, floitsch wrote:
> should not be necessary if Collection provides default implementations.
> Same below.

Done.

https://codereview.chromium.org/11931034/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/11931034/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:93: void remove(Object
object) {
Good question. I can't remember why I didn't add them here, or if there even was
a reason. I'll add them and see if anything breaks.