Convert HashSet, LinkedHashSet to factory methods and custom implementations.

Convert HashSet, LinkedHashSet to factory methods and custom implementations.

R=floitsch@google.com

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

[floitsch, 2013-09-11 11:23:17]

LGTM with comments.
I think we need to guard more for concurrent modifications.

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

https://codereview.chromium.org/23859008/diff/1/runtime/lib/collection_patch....
runtime/lib/collection_patch.dart:578: int ctr = 0;
unused variable.

https://codereview.chromium.org/23859008/diff/1/runtime/lib/collection_patch....
runtime/lib/collection_patch.dart:581: _add(object);
Since the object's equal and hashcode could have side effects we should test for
concurrent modifications.

I would override addAll in the custom set.

https://codereview.chromium.org/23859008/diff/1/runtime/lib/collection_patch....
runtime/lib/collection_patch.dart:610: void removeAll(Iterable<Object>
objectsToRemove) {
ditto.

https://codereview.chromium.org/23859008/diff/1/runtime/lib/collection_patch....
runtime/lib/collection_patch.dart:1023: var _nextEntry;
Add comment, that they are set to [this] when empty.
Explain that they have the same name as the HashSetEntry.

https://codereview.chromium.org/23859008/diff/1/sdk/lib/_internal/lib/collect...
File sdk/lib/_internal/lib/collection_patch.dart (right):

https://codereview.chromium.org/23859008/diff/1/sdk/lib/_internal/lib/collect...
sdk/lib/_internal/lib/collection_patch.dart:1181: class _CustomHashSet<E>
extends _HashSet<E> {
same as for runtime-version: needs to check for concurrent modifications when
doing addAll and removeAll.

https://codereview.chromium.org/23859008/diff/1/tests/corelib/hash_set_test.dart
File tests/corelib/hash_set_test.dart (right):

https://codereview.chromium.org/23859008/diff/1/tests/corelib/hash_set_test.d...
tests/corelib/hash_set_test.dart:27: print(set.runtimeType);
debug prints.

[Lasse Reichstein Nielsen, 2013-09-11 12:25:43]

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

https://codereview.chromium.org/23859008/diff/1/runtime/lib/collection_patch....
runtime/lib/collection_patch.dart:1023: var _nextEntry;
On 2013/09/11 11:23:17, floitsch wrote:
> Add comment, that they are set to [this] when empty.
> Explain that they have the same name as the HashSetEntry.

Done.

https://codereview.chromium.org/23859008/diff/1/sdk/lib/_internal/lib/collect...
File sdk/lib/_internal/lib/collection_patch.dart (right):

https://codereview.chromium.org/23859008/diff/1/sdk/lib/_internal/lib/collect...
sdk/lib/_internal/lib/collection_patch.dart:1181: class _CustomHashSet<E>
extends _HashSet<E> {
It needs to check for concurrent modification when doing add and remove
themselves, so it's a low-level modification.

https://codereview.chromium.org/23859008/diff/1/tests/corelib/hash_set_test.dart
File tests/corelib/hash_set_test.dart (right):

https://codereview.chromium.org/23859008/diff/1/tests/corelib/hash_set_test.d...
tests/corelib/hash_set_test.dart:27: print(set.runtimeType);
On 2013/09/11 11:23:17, floitsch wrote:
> debug prints.

Done.

[Lasse Reichstein Nielsen, 2013-09-11 12:30:02]

As discussed offline, we'll add the concurrent modification checks in a separate
CL after evaluating their performance impacts.

[Lasse Reichstein Nielsen, 2013-09-11 12:36:59]

Committed patchset #2 manually as r27378 (presubmit successful).

[Ivan Posva, 2013-09-11 17:47:48]

https://codereview.chromium.org/23859008/diff/8001/runtime/lib/collection_pat...
File runtime/lib/collection_patch.dart (right):

https://codereview.chromium.org/23859008/diff/8001/runtime/lib/collection_pat...
runtime/lib/collection_patch.dart:41: List<_HashMapEntry> _buckets = new
List<_HashMapEntry>(_INITIAL_CAPACITY);
Have you had anybody from the VM team review these changes?

[Lasse Reichstein Nielsen, 2013-09-12 05:09:35]

https://codereview.chromium.org/23859008/diff/8001/runtime/lib/collection_pat...
File runtime/lib/collection_patch.dart (right):

https://codereview.chromium.org/23859008/diff/8001/runtime/lib/collection_pat...
runtime/lib/collection_patch.dart:41: List<_HashMapEntry> _buckets = new
List<_HashMapEntry>(_INITIAL_CAPACITY);
No. 
I assume you would want to avoid the type parameter on these internal lists?

[Ivan Posva, 2013-09-24 21:16:46]

Thanks,
-Ivan

https://codereview.chromium.org/23859008/diff/8001/runtime/lib/collection_pat...
File runtime/lib/collection_patch.dart (right):

https://codereview.chromium.org/23859008/diff/8001/runtime/lib/collection_pat...
runtime/lib/collection_patch.dart:41: List<_HashMapEntry> _buckets = new
List<_HashMapEntry>(_INITIAL_CAPACITY);
On 2013/09/12 05:09:36, Lasse Reichstein Nielsen wrote:
> No. 
> I assume you would want to avoid the type parameter on these internal lists?

Yes. And it seems that they have been remove by now, correct?