Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1319783003:
Add static ensure() methods to unmodifiable views.
|
Created:
5 years, 3 months ago by nweiz Modified:
5 years, 2 months ago CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd static ensure() methods to unmodifiable views.
A common pattern in code with (largely-)immutable objects is to have a
"change" or "replace" method that constructs a new version of that
object with a few fields changed. Since the object is immutable, it
tends to use Unmodifiable views for any collections it exposes.
Creating another layer of wrapping each time the object changes is
a performance and memory issue; this change makes it much easier to
avoid re-wrapping when it's not necessary.
R=lrn@google.com
Patch Set 1 #
Total comments: 21
Patch Set 2 : Code review changes #
Messages
Total messages: 23 (4 generated)
kevmoo@google.com changed reviewers: + kevmoo@google.com
DBC: test for List, too?
LGTM if we rename the function. https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... File sdk/lib/collection/collections.dart (right): https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... sdk/lib/collection/collections.dart:20: * [UnmodifiableListView], this avoids re-wrapping it. -> , it is returned without re-wrapping it. (to specify what actually happens instead of re-wrapping). https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... sdk/lib/collection/collections.dart:22: static List ensure(Iterable source) => Move static method below constructor. https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... sdk/lib/collection/collections.dart:22: static List ensure(Iterable source) => I'm not sure I like the name. It's too ... non-descriptive. It's a verb without a subject (ensure what?). On the other hand, UnmodifiableListView.ensureUnmodifiable is quite long. I still think I'd prefer that, though, but I'm open to other suggestions. https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... sdk/lib/collection/collections.dart:23: source is UnmodifiableListBase This is slightly dangerous because someone might have extended UnmodifiableListBase, and this won't hide those extensions. On the other hand, the conditions for not being wrapped is clearly specified, so I think it's OK. https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/maps.dart File sdk/lib/collection/maps.dart (right): https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/maps.dar... sdk/lib/collection/maps.dart:217: * this avoids re-wrapping it. Also say that it returns the original object unwrapped. https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/maps.dar... sdk/lib/collection/maps.dart:219: static Map ensure(Map source) => Move below constructor, and also don't like name here. https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/maps.dar... sdk/lib/collection/maps.dart:220: source is _UnmodifiableMapMixin || source is ImmutableMap What is ImmutableMap? It looks like the VM's const map, but it doesn't exist in dart2js, so it will be a malformed type (I don't remember if that means the test throws in checked mode and succeeds in unchecked, or something else). https://codereview.chromium.org/1319783003/diff/1/tests/corelib/map_test.dart File tests/corelib/map_test.dart (right): https://codereview.chromium.org/1319783003/diff/1/tests/corelib/map_test.dart... tests/corelib/map_test.dart:126: testUnmodifiableMap(UnmodifiableMapView.ensure({1 : 37})); Also test that UnmodifiableMapView.ensure called with unmodifiable maps return the original map.
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... File sdk/lib/collection/collections.dart (right): https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... sdk/lib/collection/collections.dart:20: * [UnmodifiableListView], this avoids re-wrapping it. On 2015/08/27 07:59:04, Lasse Reichstein Nielsen wrote: > -> , it is returned without re-wrapping it. > > (to specify what actually happens instead of re-wrapping). Done. https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... sdk/lib/collection/collections.dart:22: static List ensure(Iterable source) => > I'm not sure I like the name. It's too ... non-descriptive. It's a > verb without a subject (ensure what?). > On the other hand, UnmodifiableListView.ensureUnmodifiable is quite > long. I > still think I'd prefer that, though, but I'm open to other > suggestions. My intention was that it would be clear that it would be ensuring unmodifiability, because the word "Unmodifiable" always comes before "ensure" anyway. If that doesn't resonate with you, what about "UnmodifiableListView.unmodifiable"? > Move static method below constructor. Done. https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/maps.dart File sdk/lib/collection/maps.dart (right): https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/maps.dar... sdk/lib/collection/maps.dart:217: * this avoids re-wrapping it. On 2015/08/27 07:59:04, Lasse Reichstein Nielsen wrote: > Also say that it returns the original object unwrapped. Done. https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/maps.dar... sdk/lib/collection/maps.dart:219: static Map ensure(Map source) => On 2015/08/27 07:59:04, Lasse Reichstein Nielsen wrote: > Move below constructor, and also don't like name here. Done. https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/maps.dar... sdk/lib/collection/maps.dart:220: source is _UnmodifiableMapMixin || source is ImmutableMap On 2015/08/27 07:59:04, Lasse Reichstein Nielsen wrote: > What is ImmutableMap? > It looks like the VM's const map, but it doesn't exist in dart2js, so it will be > a malformed type (I don't remember if that means the test throws in checked mode > and succeeds in unchecked, or something else). Yes, this was intended to handle constant maps. I didn't realize it was VM-only, though. Is there a reason constant Maps don't extend UnmodifiableMapBase but constant lists do extend UnmodifiableListBase? Is there any other way we can check for constant-ness? https://codereview.chromium.org/1319783003/diff/1/tests/corelib/map_test.dart File tests/corelib/map_test.dart (right): https://codereview.chromium.org/1319783003/diff/1/tests/corelib/map_test.dart... tests/corelib/map_test.dart:126: testUnmodifiableMap(UnmodifiableMapView.ensure({1 : 37})); On 2015/08/27 07:59:04, Lasse Reichstein Nielsen wrote: > Also test that UnmodifiableMapView.ensure called with unmodifiable maps return > the original map. It does that in the body of [testUnmodifiableMap] below.
Code review changes
sra@google.com changed reviewers: + sra@google.com
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... File sdk/lib/collection/collections.dart (right): https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... sdk/lib/collection/collections.dart:22: static List ensure(Iterable source) => How does this type-check in DDC?
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... File sdk/lib/collection/collections.dart (right): https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... sdk/lib/collection/collections.dart:22: static List ensure(Iterable source) => On 2015/08/27 20:24:52, sra1 wrote: > How does this type-check in DDC? I'm not sure how to test that... "dartanalyzer --strong sdk/lib/collection/collection.dart" reports a whole bunch of independent errors.
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... File sdk/lib/collection/collections.dart (right): https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... sdk/lib/collection/collections.dart:22: static List ensure(Iterable source) => UmodifiableListView.unmodifiable isn't very clear about what it does, and seems redundant. UnmidifiableListView.ensure is a verb and it's not clear what it's ensuring (not even when Unmidifiable is before it, so is List and View, and it's still hanging). I think I prefer ensureUnmodifiable. Alterantives like unmodifiableIfNot or ifModifiable are not great either, with the latter *maybe* being reasonable - it's just too far from everything else we do.
rnystrom@google.com changed reviewers: + rnystrom@google.com
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... File sdk/lib/collection/collections.dart (right): https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... sdk/lib/collection/collections.dart:22: static List ensure(Iterable source) => On 2015/08/27 07:59:04, Lasse Reichstein Nielsen wrote: > I'm not sure I like the name. It's too ... non-descriptive. It's a verb without > a subject (ensure what?). > > On the other hand, UnmodifiableListView.ensureUnmodifiable is quite long. I > still think I'd prefer that, though, but I'm open to other suggestions. Drive-by comment! A 39-character method name is pretty rough for a call that often appears inside constructor initialization lists. Personally, I think "ensure" is pretty unambiguous because of the class where it appears. Given that it's on UnmodifiableListView, it takes a list, and returns an UnmodifiableListView, what else could it be ensuring? Another option would to make a class like this in package:collection: class Unmodifiable { static UnmodifiableListView list(List list) { ... } static UnmodifiableMapView list(Map map) { ... } static UnmodifiableSetView list(Set set) { ... } } Letting you do: _foo = Unmodifiable.list(foo); You do lose the type argument, though. :( It's a shame the constructors for UnmodifiableListView et. al. can't just be factory constructors that always do this.
On 2015/08/31 20:42:45, Bob Nystrom wrote: > https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... > File sdk/lib/collection/collections.dart (right): > > https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... > sdk/lib/collection/collections.dart:22: static List ensure(Iterable source) => > On 2015/08/27 07:59:04, Lasse Reichstein Nielsen wrote: > > I'm not sure I like the name. It's too ... non-descriptive. It's a verb > without > > a subject (ensure what?). > > > > On the other hand, UnmodifiableListView.ensureUnmodifiable is quite long. I > > still think I'd prefer that, though, but I'm open to other suggestions. > > Drive-by comment! A 39-character method name is pretty rough for a call that > often appears inside constructor initialization lists. > > Personally, I think "ensure" is pretty unambiguous because of the class where it > appears. Given that it's on UnmodifiableListView, it takes a list, and returns > an UnmodifiableListView, what else could it be ensuring? > > Another option would to make a class like this in package:collection: > > class Unmodifiable { > static UnmodifiableListView list(List list) { ... } > static UnmodifiableMapView list(Map map) { ... } > static UnmodifiableSetView list(Set set) { ... } > } > > Letting you do: > > _foo = Unmodifiable.list(foo); > > You do lose the type argument, though. :( > > It's a shame the constructors for UnmodifiableListView et. al. can't just be > factory constructors that always do this. I kind of like the Unmodifiable class idea. One note, though: it would have to just return "List", "Map", "Set" etc to handle cases like const lists/maps and lists extending UnmodifiableListBase.
sra@google.com changed reviewers: + leafp@google.com
+leaf to address if this is a problem for DDC.
On 2015/08/31 23:22:44, sra1 wrote: > +leaf to address if this is a problem for DDC. I don't think it's more of a problem than any of the other non-generic generic methods. I don't think DDC will have any problem typing the code that the CL adds, unless I'm missing something about the relationships of the various types. However, the result of calling ensure won't be castable back to a List<T> for any concrete type T in the case where it actually introduced a wrapper. Depending on how the code is written, you'd either get a static error, or a runtime cast failure when you tried to turn the List back into a List<T>. So long term, DDC will need to make "ensure" a generic function for this to work.
On 2015/08/31 23:59:30, Leaf wrote: > On 2015/08/31 23:22:44, sra1 wrote: > > +leaf to address if this is a problem for DDC. > > I don't think it's more of a problem than any of the other non-generic generic > methods. I don't think DDC will have any problem typing the code that the CL > adds, unless I'm missing something about the relationships of the various types. > However, the result of calling ensure won't be castable back to a List<T> for > any concrete type T in the case where it actually introduced a wrapper. > Depending on how the code is written, you'd either get a static error, or a > runtime cast failure when you tried to turn the List back into a List<T>. So > long term, DDC will need to make "ensure" a generic function for this to work. This is guaranteed to fail in DDC, right? List<int> xs = UnmodifiableListView.ensure(<int>[1]); This might fail and might not, depending on the implementation of const lists: List<int> cs = UnmodifiableListView.ensure(const <int>[1]);
Why can't we change the constructors to do this?
class UnmodifiableListView<E> ... {
UnmodifiableListView._(this._source);
factory UnmodifiableListView(Iterable<E> source) =>
source is UnmodifiableListView<E>
? source
: new UmodifiableListView<E>._(source);
}
Given the tricky issues with VM vs dart2js, we clearly need List tests.
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti...
File sdk/lib/collection/collections.dart (right):
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti...
sdk/lib/collection/collections.dart:23: source is UnmodifiableListBase
On 2015/08/27 07:59:04, Lasse Reichstein Nielsen wrote:
> This is slightly dangerous because someone might have extended
> UnmodifiableListBase, and this won't hide those extensions.
> On the other hand, the conditions for not being wrapped is clearly specified,
so
> I think it's OK.
I don't think it is clear because it is not clear which classes are implemented
using UnmodifiableListBase.
VM runtime and JS runtime don't use UnmodifiableListBase the same way.
The VM's private _ImmutableList, used for constants, extends
UnmodifiableListBase.
dart2js const lists to not satisfy 'x is UnmodifiableListBase' and it is not
easily to make then do so because there is really only one class for const and
non-const lists - JavaScript Array.
UnmodifiableListView.ensure(new List.unmodifiable(source))
UnmodifiableListView.ensure(const [])
will be wrapped unpredictably (currently on one implementation and not in the
other.)
If the test was for UnmodifiableListView, the constructor could be changed to a
factory constructor (and keep the type arguments) rather than a separate ensure
method. This would solve the DDC problems.
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti...
sdk/lib/collection/collections.dart:28: UnmodifiableListView(Iterable<E> source)
: _source = source;
Question: why is source Iterable?
On 2015/10/06 19:19:22, sra1 wrote:
> Why can't we change the constructors to do this?
>
>
> class UnmodifiableListView<E> ... {
>
> UnmodifiableListView._(this._source);
>
> factory UnmodifiableListView(Iterable<E> source) =>
> source is UnmodifiableListView<E>
> ? source
> : new UmodifiableListView<E>._(source);
> }
>
>
> Given the tricky issues with VM vs dart2js, we clearly need List tests.
This has a *lot* of false positives for sources that need to be wrapped. It
doesn't support implementations of UnmodifiableListBase (which includes const
lists on the VM), and "is" checks on reified generics are unreliable enough that
it will miss some valid UnmodifiableListViews as well.
>
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti...
> File sdk/lib/collection/collections.dart (right):
>
>
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti...
> sdk/lib/collection/collections.dart:23: source is UnmodifiableListBase
> On 2015/08/27 07:59:04, Lasse Reichstein Nielsen wrote:
> > This is slightly dangerous because someone might have extended
> > UnmodifiableListBase, and this won't hide those extensions.
> > On the other hand, the conditions for not being wrapped is clearly
specified,
> so
> > I think it's OK.
>
> I don't think it is clear because it is not clear which classes are
implemented
> using UnmodifiableListBase.
>
> VM runtime and JS runtime don't use UnmodifiableListBase the same way.
> The VM's private _ImmutableList, used for constants, extends
> UnmodifiableListBase.
> dart2js const lists to not satisfy 'x is UnmodifiableListBase' and it is not
> easily to make then do so because there is really only one class for const and
> non-const lists - JavaScript Array.
>
> UnmodifiableListView.ensure(new List.unmodifiable(source))
> UnmodifiableListView.ensure(const [])
>
> will be wrapped unpredictably (currently on one implementation and not in the
> other.)
This seems like a feature to me. I want this function to do as little work as
possible—if it can tell that the list is unmodifiable on some platforms, I want
it to avoid wrapping on those platforms. That's way better than just always
doing extra work for const lists in the name of consistency.
> If the test was for UnmodifiableListView, the constructor could be changed to
a
> factory constructor (and keep the type arguments) rather than a separate
ensure
> method. This would solve the DDC problems.
>
>
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti...
> sdk/lib/collection/collections.dart:28: UnmodifiableListView(Iterable<E>
source)
> : _source = source;
> Question: why is source Iterable?
On 2015/10/06 20:47:12, nweiz wrote:
> On 2015/10/06 19:19:22, sra1 wrote:
> > Why can't we change the constructors to do this?
> >
> >
> > class UnmodifiableListView<E> ... {
> >
> > UnmodifiableListView._(this._source);
> >
> > factory UnmodifiableListView(Iterable<E> source) =>
> > source is UnmodifiableListView<E>
> > ? source
> > : new UmodifiableListView<E>._(source);
> > }
> >
> >
> > Given the tricky issues with VM vs dart2js, we clearly need List tests.
>
> This has a *lot* of false positives for sources that need to be wrapped.
It
> doesn't support implementations of UnmodifiableListBase (which includes const
> lists on the VM), and "is" checks on reified generics are unreliable enough
that
> it will miss some valid UnmodifiableListViews as well.
Which valid ones will it miss?
>
> >
>
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti...
> > File sdk/lib/collection/collections.dart (right):
> >
> >
>
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti...
> > sdk/lib/collection/collections.dart:23: source is UnmodifiableListBase
> > On 2015/08/27 07:59:04, Lasse Reichstein Nielsen wrote:
> > > This is slightly dangerous because someone might have extended
> > > UnmodifiableListBase, and this won't hide those extensions.
> > > On the other hand, the conditions for not being wrapped is clearly
> specified,
> > so
> > > I think it's OK.
> >
> > I don't think it is clear because it is not clear which classes are
> implemented
> > using UnmodifiableListBase.
> >
> > VM runtime and JS runtime don't use UnmodifiableListBase the same way.
> > The VM's private _ImmutableList, used for constants, extends
> > UnmodifiableListBase.
> > dart2js const lists to not satisfy 'x is UnmodifiableListBase' and it is not
> > easily to make then do so because there is really only one class for const
and
> > non-const lists - JavaScript Array.
> >
> > UnmodifiableListView.ensure(new List.unmodifiable(source))
> > UnmodifiableListView.ensure(const [])
> >
> > will be wrapped unpredictably (currently on one implementation and not in
the
> > other.)
>
> This seems like a feature to me. I want this function to do as little work as
> possible—if it can tell that the list is unmodifiable on some platforms, I
want
> it to avoid wrapping on those platforms. That's way better than just always
> doing extra work for const lists in the name of consistency.
If we could stomach the change it would be better to add
class Iterable<E> ... {
List<E> toUnmodifiableList(); // => new List<E>.unmodifiable(this);
List<E> toUnmodifiableListView(); // => new UnmodifiableListView<E>(this);
}
Then each implementation could return itself if unmodifiable, and there is no
loss of debugging benefit for checked mode.
If we can't maintain the type parameter, then I prefer Bob's suggestion of a
separate class (or better, library).
We need to stop adding things to corelib that don't work with DDC.
>
> > If the test was for UnmodifiableListView, the constructor could be changed
to
> a
> > factory constructor (and keep the type arguments) rather than a separate
> ensure
> > method. This would solve the DDC problems.
> >
> >
>
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti...
> > sdk/lib/collection/collections.dart:28: UnmodifiableListView(Iterable<E>
> source)
> > : _source = source;
> > Question: why is source Iterable?
https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... File sdk/lib/collection/collections.dart (right): https://codereview.chromium.org/1319783003/diff/1/sdk/lib/collection/collecti... sdk/lib/collection/collections.dart:28: UnmodifiableListView(Iterable<E> source) : _source = source; Because the UnmodifiableListView is also a way to make an Iterable with efficient length and elementAt accessible as a List without calling toList on it.
DBQ Could we just update the 'unmodifiable' ctors on List and Map to return 'other' if it is unmodifiable? We can keep the generic. We don't introduce a new API.
On 2015/10/20 15:27:50, kevmoo wrote:
> DBQ
>
> Could we just update the 'unmodifiable' ctors on List and Map to return
'other'
> if it is unmodifiable?
>
> We can keep the generic. We don't introduce a new API.
If you want to be nitty about it (which we usually are for core library
changes), this would be a breaking change. The following code changes behavior:
var l1 = [];
var l2 = new UnmodifiableListView(l1);
var l3 = new List.unmodifiable(l2);
l1.add(1);
print(l3);
This currently prints "[]" because the unmodifiable list copies its input, but
under the proposed semantics it would print "[1]". It's not a huge stretch to
imagine that there's code that relies on the copying behavior.
This is why I put the function on UnmodifiableListView/UnmodifiableMapView in
the first place—that way it's clear that it returns a *view*, not a copy.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
