Add CombinedMapView #53
Conversation
| @override | ||
| V operator [](Object key) { | ||
| for (var map in _maps) { | ||
| // Avoid two hash look ups on a positive hit. |
|
I'll rebase against master and ping when ready. |
|
Ready for review @nex3! |
| class CombinedMapView<K, V> extends UnmodifiableMapBase<K, V> { | ||
| final Iterable<Map<K, V>> _maps; | ||
|
|
||
| CombinedMapView(this._maps); |
There was a problem hiding this comment.
Document that the maps iterable is accessed repeatedly, so it should be a collection type like List or Set rather than a lazy iterable produced by map() et al.
I really wish we had a distinct Collection type so we could express that statically.
| return null; | ||
| } | ||
|
|
||
| Iterable<K> get keys => new CombinedIterableView<K>(_maps.map((m) => m.keys)); |
There was a problem hiding this comment.
From the UnmodifiableMapBase docs:
The
keysiterable should have efficientlengthandcontainsoperations, and it should catch concurrent modifications of the keys while iterating.
CombinedIterableView doesn't have an efficient contains() (maybe that should be special-cased as well). It also won't necessarily throw if a later map is modified while an earlier one is being traversed, but that may not realistically be fixable.
There was a problem hiding this comment.
I've added a special case for contains in CombinedIterableView.
I don't think I can do anything about concurrent modifications. I can document that concurrent modifications should be avoided because we don't throw?
There was a problem hiding this comment.
Documenting that would be good. Be sure to mention that it will sometimes throw, but maybe don't guarantee the specific circumstances so we can change it in the future if we need to.
|
|
||
| V operator [](Object key) { | ||
| for (var map in _maps) { | ||
| // Avoid two hash look ups on a positive hit. |
test/combined_wrapper/map_test.dart
Outdated
| final map1 = const {1: 1, 2: 2, 3: 3}; | ||
| final map2 = const {4: 4, 5: 5, 6: 6}; | ||
| final map3 = const {7: 7, 8: 8, 9: 9}; | ||
| final concat = {}..addAll(map1)..addAll(map2)..addAll(map3); |
| @@ -24,6 +24,8 @@ class CombinedIterableView<T> extends IterableBase<T> { | |||
| // Special cased isEmpty/length since many iterables have an efficient | |||
There was a problem hiding this comment.
Mention that contains is special-cased as well.
| return null; | ||
| } | ||
|
|
||
| Iterable<K> get keys => new CombinedIterableView<K>(_maps.map((m) => m.keys)); |
There was a problem hiding this comment.
Documenting that would be good. Be sure to mention that it will sometimes throw, but maybe don't guarantee the specific circumstances so we can change it in the future if we need to.
|
One more tiny ping for @kevmoo |
* Add CombinedMapView. * Mistype. * Address feedback. * Address feedback.
Branched from #52, which needs to be merged first.
Finally closes dart-lang/core#711.