|
|
Created:
4 years, 8 months ago by nweiz Modified:
4 years, 7 months ago CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/collection@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd GroupSet and SetGroup classes.
GroupSet provides a view of the union of a set of sets. SetGroup
provides an easy way to manage a GroupSet in a class context.
R=floitsch@google.com, lrn@google.com
Committed: https://github.com/dart-lang/collection/commit/e3a6fca00c0c0d732a714941a92b6052d211e7bb
Patch Set 1 #Patch Set 2 : remove dev version #Patch Set 3 : a few more docs #
Total comments: 7
Patch Set 4 : Add a disjoint parameter to SetGroup. #
Total comments: 6
Patch Set 5 : Code review changes #Patch Set 6 : Code review changes #Patch Set 7 : Code review changes #
Total comments: 36
Patch Set 8 : Code review changes #
Total comments: 5
Patch Set 9 : Code review changes #
Messages
Total messages: 37 (2 generated)
https://codereview.chromium.org/1873373002/diff/40001/lib/src/group_set.dart File lib/src/group_set.dart (right): https://codereview.chromium.org/1873373002/diff/40001/lib/src/group_set.dart#... lib/src/group_set.dart:15: /// in the earliest inner set is preferred. Worth mentioning that the individual sets are assumed to use operator= equality. If they don't, the GroupSet may not have a consistent behavior. https://codereview.chromium.org/1873373002/diff/40001/lib/src/group_set.dart#... lib/src/group_set.dart:30: /// many operations including [length] more efficient. .. and if the sets turn out to not be disjoint after all, some operations may behave inconsistently. https://codereview.chromium.org/1873373002/diff/40001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/40001/lib/src/set_group.dart#... lib/src/set_group.dart:17: /// I can't figure out when I would use this class, and the example doesn't help me. Can you expand on the example? Is it really just a shorthand for: var sets = new Set<Set<E>>(); var set = new GroupSet<E>(sets); where you make "set" a getter on sets so you can pass it around as one value instead of as two? If so, I think it's too specialized to warrant a separate class, and extending the Set<Set<E>> interface is something I'd try to avoid. I'd rather have a "SetGroup" which is a pair of Set<Set> and GroupSet, and not embed one of them in the other and invent a new almost-set interface. https://codereview.chromium.org/1873373002/diff/60001/lib/src/group_set.dart File lib/src/group_set.dart (right): https://codereview.chromium.org/1873373002/diff/60001/lib/src/group_set.dart#... lib/src/group_set.dart:9: /// A single set that provides a view of a set of sets. Why must it be a set of sets and not just any collection of sets? I.e., why Set<Set> and not Iterable<Set>? All the code below looks like it would work anyway, and you avoid the extra .toSet in the constructor. If a user wants to de-duplicate the input they can call toSet before passing it to the GroupSet constructor. https://codereview.chromium.org/1873373002/diff/60001/lib/src/group_set.dart#... lib/src/group_set.dart:31: GroupSet(this._sets, {bool disjoint: false}) : _disjoint = disjoint; As usual, I recommend not having a private parameter name in the exposed API. https://codereview.chromium.org/1873373002/diff/60001/lib/src/group_set.dart#... lib/src/group_set.dart:53: Consider moving the following code into a separate helper function. That increases the likelihood that the function can be inlined in the _disjoint case.
Code review changes
https://codereview.chromium.org/1873373002/diff/40001/lib/src/group_set.dart File lib/src/group_set.dart (right): https://codereview.chromium.org/1873373002/diff/40001/lib/src/group_set.dart#... lib/src/group_set.dart:15: /// in the earliest inner set is preferred. On 2016/04/12 13:55:22, Lasse Reichstein Nielsen wrote: > Worth mentioning that the individual sets are assumed to use operator= equality. > If they don't, the GroupSet may not have a consistent behavior. Done. We could have the weaker requirement that the component sets have the same equality semantics, but we'd have to do something like `_sets.first.toSet()..clear()` in _iterable and that doesn't seem great. https://codereview.chromium.org/1873373002/diff/40001/lib/src/group_set.dart#... lib/src/group_set.dart:30: /// many operations including [length] more efficient. On 2016/04/12 13:55:22, Lasse Reichstein Nielsen wrote: > .. and if the sets turn out to not be disjoint after all, some operations may > behave inconsistently. Done. https://codereview.chromium.org/1873373002/diff/40001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/40001/lib/src/set_group.dart#... lib/src/set_group.dart:17: /// On 2016/04/12 13:55:22, Lasse Reichstein Nielsen wrote: > I can't figure out when I would use this class, and the example doesn't help me. > Can you expand on the example? > > Is it really just a shorthand for: > > var sets = new Set<Set<E>>(); > var set = new GroupSet<E>(sets); > > where you make "set" a getter on sets so you can pass it around as one value > instead of as two? > > If so, I think it's too specialized to warrant a separate class, and extending > the Set<Set<E>> interface is something I'd try to avoid. I'd rather have a > "SetGroup" which is a pair of Set<Set> and GroupSet, and not embed one of them > in the other and invent a new almost-set interface. Done.
Code review changes
https://codereview.chromium.org/1873373002/diff/40001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/40001/lib/src/set_group.dart#... lib/src/set_group.dart:17: /// On 2016/04/12 20:29:20, nweiz wrote: > On 2016/04/12 13:55:22, Lasse Reichstein Nielsen wrote: > > I can't figure out when I would use this class, and the example doesn't help > me. > > Can you expand on the example? > > > > Is it really just a shorthand for: > > > > var sets = new Set<Set<E>>(); > > var set = new GroupSet<E>(sets); > > > > where you make "set" a getter on sets so you can pass it around as one value > > instead of as two? > > > > If so, I think it's too specialized to warrant a separate class, and extending > > the Set<Set<E>> interface is something I'd try to avoid. I'd rather have a > > "SetGroup" which is a pair of Set<Set> and GroupSet, and not embed one of them > > in the other and invent a new almost-set interface. > > Done. After making the change and trying it out, I remembered why this is important: the code snippet you wrote doesn't work. One instance variable can't refer to another in its initializer, so "new GroupSet(sets)" doesn't work. We need some sort of utility class to gracefully initialize this. I've expanded the example to be a little clearer about how this is supposed to be used. I think it does make sense to have this implement Set<Set<E>> directly. I'm thinking of it as an analogue to StreamGroup or FutureGroup in the async package, which are in turn analogues to StreamController and Completer: they're "backend" classes that provide an API for controlling the behavior of a "frontend" class that's exposed to the user. Set APIs like add() and remove() are part of that backend API. If you think it's too weird to expose the full set API, I'd rather just support add() and remove() explicitly. This seemed needlessly limited to me, but I think it's a better API than requiring users to write "group.components.add" or whatever.
On 2016/04/13 00:00:57, nweiz wrote: > After making the change and trying it out, I remembered why this is important: > the code snippet you wrote doesn't work. One instance variable can't refer to > another in its initializer, so "new GroupSet(sets)" doesn't work. We need some > sort of utility class to gracefully initialize this. That's a general problem and adding and extra class per instance isn't the best way to solve it. In cases like this, I would use constructors, maybe more than one if necessary: Either: class Something<E> { final Set<Set<E>> sets; final Set<E> union; factory Something() { var sets = new Set<Set<E>>(); var set = new GroupSet<E>(sets); return new Something<E>._(sets, set); } Something._(this.sets, this.union); } or, if it can't be a factory constructor: class Something<E> { final Set<Set<E>> sets; final Set<E> union; Something() : this._(new Set<Set<E>>()); Something._(var sets) : sets = sets, union = new GroupSet(sets); } It is annoying that you can't declare variables local to an initializer list, but using an extra constructor is a way around that. I don't think a class for a Set + an extra field is a good model. I'd rather have a pair of Set and Set<Set> that you can extract both from instead of putting one on the other for convenience. > I've expanded the example to be a little clearer about how this is supposed to > be used. I think it does make sense to have this implement Set<Set<E>> directly. > I'm thinking of it as an analogue to StreamGroup or FutureGroup in the async > package, which are in turn analogues to StreamController and Completer: they're > "backend" classes that provide an API for controlling the behavior of a > "frontend" class that's exposed to the user. Set APIs like add() and remove() > are part of that backend API. > > If you think it's too weird to expose the full set API, I'd rather just support > add() and remove() explicitly. This seemed needlessly limited to me, but I think > it's a better API than requiring users to write "group.components.add" or > whatever. There is a point in this, and it might be confusing me that the type of the "controller" is also the type of the wrapped object (Set<Set>) - and it is in fact the same object. I'll have a look at it again. (I also had a few more comments that was added to patch set 4 instead of patch set 3).
On 2016/04/13 09:40:15, Lasse Reichstein Nielsen wrote: > On 2016/04/13 00:00:57, nweiz wrote: > > > After making the change and trying it out, I remembered why this is important: > > the code snippet you wrote doesn't work. One instance variable can't refer to > > another in its initializer, so "new GroupSet(sets)" doesn't work. We need some > > sort of utility class to gracefully initialize this. > > That's a general problem and adding and extra class per instance isn't the best > way to solve it. I agree that this is a general issue, and the moment there's language support for doing something better I'll deprecate this class. But I don't think that's on the horizon, so for now it makes sense to code towards the language we have. > In cases like this, I would use constructors, maybe more than one if necessary: > > Either: > > class Something<E> { > final Set<Set<E>> sets; > final Set<E> union; > factory Something() { > var sets = new Set<Set<E>>(); > var set = new GroupSet<E>(sets); > return new Something<E>._(sets, set); > } > Something._(this.sets, this.union); > } > > or, if it can't be a factory constructor: > > class Something<E> { > final Set<Set<E>> sets; > final Set<E> union; > Something() : this._(new Set<Set<E>>()); > Something._(var sets) : sets = sets, union = new GroupSet(sets); > } > > It is annoying that you can't declare variables local to an initializer list, > but > using an extra constructor is a way around that. This is an egregious amount of extra work, and it makes code a *lot* harder to read. In a large real-world class with a lot of changing state, there's a huge amount of value in having the declaration and definition of fields in one place, especially when multiple fields/getters have definitions that rely on one another. Having to jump between the definitions and the constructor to figure out what's going on makes the class much harder to understand, and is unnecessary when the fields don't have anything to do with constructor parameters. Having to duplicate the constructor to simulate local variables in initializers makes it even more gross. > I don't think a class for a Set + an extra field is a good model. > I'd rather have a pair of Set and Set<Set> that you can extract both from > instead > of putting one on the other for convenience. > > > I've expanded the example to be a little clearer about how this is supposed to > > be used. I think it does make sense to have this implement Set<Set<E>> > directly. > > I'm thinking of it as an analogue to StreamGroup or FutureGroup in the async > > package, which are in turn analogues to StreamController and Completer: > they're > > "backend" classes that provide an API for controlling the behavior of a > > "frontend" class that's exposed to the user. Set APIs like add() and remove() > > are part of that backend API. > > > > If you think it's too weird to expose the full set API, I'd rather just > support > > add() and remove() explicitly. This seemed needlessly limited to me, but I > think > > it's a better API than requiring users to write "group.components.add" or > > whatever. > > There is a point in this, and it might be confusing me that the type of the > "controller" is also the type of the wrapped object (Set<Set>) - and it is in > fact > the same object. I'll have a look at it again. I'm okay with making the controller not implement Set if you think that's better. > (I also had a few more comments that was added to patch set 4 instead of patch > set 3).
Code review changes
https://codereview.chromium.org/1873373002/diff/60001/lib/src/group_set.dart File lib/src/group_set.dart (right): https://codereview.chromium.org/1873373002/diff/60001/lib/src/group_set.dart#... lib/src/group_set.dart:9: /// A single set that provides a view of a set of sets. On 2016/04/12 13:55:22, Lasse Reichstein Nielsen wrote: > Why must it be a set of sets and not just any collection of sets? > I.e., why Set<Set> and not Iterable<Set>? > All the code below looks like it would work anyway, and you avoid the extra > .toSet in the constructor. If a user wants to de-duplicate the input they can > call toSet before passing it to the GroupSet constructor. Supporting any iterable would be confusing. APIs that take iterables pretty much universally only iterate over them once in case they're lazily generated from some expensive or side-effect-y computation, so taking an iterable with the intent to iterate over it repeatedly would break user expectations. Given that, the choice is what concrete collection type to use. Set seems logical since duplicating an element doesn't change the semantics. https://codereview.chromium.org/1873373002/diff/60001/lib/src/group_set.dart#... lib/src/group_set.dart:31: GroupSet(this._sets, {bool disjoint: false}) : _disjoint = disjoint; On 2016/04/12 13:55:22, Lasse Reichstein Nielsen wrote: > As usual, I recommend not having a private parameter name in the exposed API. Acknowledged, but this is a very common pattern in Dart code and it doesn't seem to cause problems in practice. https://codereview.chromium.org/1873373002/diff/60001/lib/src/group_set.dart#... lib/src/group_set.dart:53: On 2016/04/12 13:55:22, Lasse Reichstein Nielsen wrote: > Consider moving the following code into a separate helper function. > That increases the likelihood that the function can be inlined in the _disjoint > case. Done.
On 2016/04/13 21:30:06, nweiz wrote: > https://codereview.chromium.org/1873373002/diff/60001/lib/src/group_set.dart > File lib/src/group_set.dart (right): > > https://codereview.chromium.org/1873373002/diff/60001/lib/src/group_set.dart#... > lib/src/group_set.dart:9: /// A single set that provides a view of a set of > sets. > On 2016/04/12 13:55:22, Lasse Reichstein Nielsen wrote: > > Why must it be a set of sets and not just any collection of sets? > > I.e., why Set<Set> and not Iterable<Set>? > > All the code below looks like it would work anyway, and you avoid the extra > > .toSet in the constructor. If a user wants to de-duplicate the input they can > > call toSet before passing it to the GroupSet constructor. > > Supporting any iterable would be confusing. APIs that take iterables pretty much > universally only iterate over them once in case they're lazily generated from > some expensive or side-effect-y computation, so taking an iterable with the > intent to iterate over it repeatedly would break user expectations. That's actually a problem we have in general - Iterable is the only common superclass of collections (List, Set) and it's sometimes used as that, and in other cases it's used as a way to pass a computed iteration of values that will only be iterated once. We don't have a distinction, and we generally can't promise that we won't iterate an iterable more than once (we just try to avoid it since it keeps breaking badly written code). > Given that, the choice is what concrete collection type to use. Since we don't have Collection as a class :( > Set seems > logical since duplicating an element doesn't change the semantics. Actually, I get to the opposite position from the same premise - since duplicates won't change semantics, it is unnecessary to enforce lack of duplicates. Sets are more complex than lists, so the simplest choice is List. The most "correct" class is just Iterable - as long as it's clear that it will be iterated repeatedly. I can see that it might cause confusion, though, but I'd be willing to risk it. > > https://codereview.chromium.org/1873373002/diff/60001/lib/src/group_set.dart#... > lib/src/group_set.dart:31: GroupSet(this._sets, {bool disjoint: false}) : > _disjoint = disjoint; > On 2016/04/12 13:55:22, Lasse Reichstein Nielsen wrote: > > As usual, I recommend not having a private parameter name in the exposed API. > > Acknowledged, but this is a very common pattern in Dart code and it doesn't seem > to cause problems in practice. > > https://codereview.chromium.org/1873373002/diff/60001/lib/src/group_set.dart#... > lib/src/group_set.dart:53: > On 2016/04/12 13:55:22, Lasse Reichstein Nielsen wrote: > > Consider moving the following code into a separate helper function. > > That increases the likelihood that the function can be inlined in the > _disjoint > > case. > > Done.
Ping? This is blocking some work in test that an internal customer is waiting on.
+floitsch Ack, sorry. We're still disagreeing on some of the choices, but it's mainly a matter of taste, so let's get a third opinion.
lrn@google.com changed reviewers: + floitsch@google.com
https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { I do think I'd prefer this class to not be a Set. I can still have add, remove and contains methods, but probably nothing else. It may have a Set getter (Set<Set<E>> get sets) that gives a Set view, but the class itself should not be a Set - it's role is to control the GroupSet, not to *be* a Set. That also avoids extending the Set interface.
Please consider changing the names. GroupSet and SetGroup are not used commonly enough to know which one is which. Even worse, the SetGroup is a Set (and not a Group). I'm not sure the GroupSet is necessary. I would just add two more methods to `GroupSet`. https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart File lib/src/group_set.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:17: class GroupSet<E> extends SetMixin<E> with UnmodifiableSetMixin<E> { What about "UnionSet" ? https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:19: final Set<Set<E>> _sets; I'm not sure I like the fact that this has to be a Set. There are few advantages of making this a 'Set'. List seems to be more efficient, although that would preclude an Iterable here. https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:26: /// If any sets in [sets] change, this [GroupSet] will reflect that change. If reflects that change. ... reflects ... https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:29: /// If [disjoint] is `true`, this assumes that all component sets are If [disjoint] is `true`, then all component sets must be disjoint. That is, they must not contain any element in common. This makes ... I wouldn't even mention "inconsistent" behavior. https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:30: /// disjoint—that is, that they contain no elements in common. This makes many Avoid non-ascii characters in documentation. https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:37: /// If any sets in [sets] change, this [GroupSet] will reflect that change. reflects https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:39: /// changes in [sets] will not be reflected in this [GroupSet]. are not reflected https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:41: /// If [disjoint] is `true`, this assumes that all component sets are as above. https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { Do we even need this class? I see two ways to avoid it: Add 'addSet', 'removeSet' to the `GroupSet`. Keep the set of sets that are used to initialize the `GroupSet`. class Engine { Set<Test> get activeTests => _activeTestsGroup.set; final Set<Set<Test>> _activeTestSets = new Set<Set<Test>>(); get _activeTestsGroup = new SetGroup<Test>(_activeTestSets); void addSuite(Suite suite) { _activeTestSets.add(suite.tests); _runSuite(suite); _activeTestSets.remove(suite.tests); } } In any case, the naming is very confusing. https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:28: GroupSet<E> _set; This should be final.
Code review changes
https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart File lib/src/group_set.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:17: class GroupSet<E> extends SetMixin<E> with UnmodifiableSetMixin<E> { On 2016/04/25 16:11:09, floitsch wrote: > What about "UnionSet" ? I like it. Done. https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:19: final Set<Set<E>> _sets; On 2016/04/25 16:11:10, floitsch wrote: > I'm not sure I like the fact that this has to be a Set. > > There are few advantages of making this a 'Set'. > List seems to be more efficient, although that would preclude an Iterable here. I discussed this with lrn earlier. Ideally this would just be Collection to allow either List or Set, but that type doesn't exist. Making it a set provides automatic de-duplication and O(1) removal of the component sets without an algorithmic cost for the main [GroupSet] operations. https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:26: /// If any sets in [sets] change, this [GroupSet] will reflect that change. If On 2016/04/25 16:11:10, floitsch wrote: > reflects that change. ... reflects ... Done. https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:29: /// If [disjoint] is `true`, this assumes that all component sets are On 2016/04/25 16:11:09, floitsch wrote: > If [disjoint] is `true`, then all component sets must be disjoint. That is, they > must not contain any element in common. This makes ... Done. > I wouldn't even mention "inconsistent" behavior. Haha, this is exactly the opposite of Lasse's feedback. I think it's reasonable to mention, though, since otherwise users might expect this to produce an error. https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:30: /// disjoint—that is, that they contain no elements in common. This makes many On 2016/04/25 16:11:09, floitsch wrote: > Avoid non-ascii characters in documentation. Done, although I don't think this is a good general rule—it's 2016 and pretty much everything can display Unicode effectively. If this should be a rule, it should be mentioned in Effective Dart. https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:37: /// If any sets in [sets] change, this [GroupSet] will reflect that change. On 2016/04/25 16:11:10, floitsch wrote: > reflects Done. https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:39: /// changes in [sets] will not be reflected in this [GroupSet]. On 2016/04/25 16:11:10, floitsch wrote: > are not reflected Done. https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:41: /// If [disjoint] is `true`, this assumes that all component sets are On 2016/04/25 16:11:10, floitsch wrote: > as above. Done. https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { On 2016/04/23 09:00:47, Lasse Reichstein Nielsen wrote: > I do think I'd prefer this class to not be a Set. > > I can still have add, remove and contains methods, but probably nothing else. > It may have a Set getter (Set<Set<E>> get sets) that gives a Set view, but the > class itself should not be a Set - it's role is to control the GroupSet, not to > *be* a Set. That also avoids extending the Set interface. Done. > Add 'addSet', 'removeSet' to the `GroupSet`. It's important that GroupSet *not* have additional methods that aren't in Set. The fact that a Set exposed by a class is implemented using GroupSet should be invisible to the user. > Keep the set of sets that are used to initialize the `GroupSet`. Creating a new SetGroup for every call to [activeTests] is a lot of overhead and indirection that would be avoided by this class existing. https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:28: GroupSet<E> _set; On 2016/04/25 16:11:10, floitsch wrote: > This should be final. Unfortunately it can't be; GroupSet's initializer refers to [this].
Almost there... https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart File lib/src/group_set.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:29: /// If [disjoint] is `true`, this assumes that all component sets are On 2016/04/26 21:23:52, nweiz wrote: > On 2016/04/25 16:11:09, floitsch wrote: > > If [disjoint] is `true`, then all component sets must be disjoint. That is, > they > > must not contain any element in common. This makes ... > > Done. > > > I wouldn't even mention "inconsistent" behavior. > > Haha, this is exactly the opposite of Lasse's feedback. I think it's reasonable > to mention, though, since otherwise users might expect this to produce an error. Imho, it's the change to "must" that makes the 'inconsistent behavior' unnecessary. https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { On 2016/04/26 21:23:53, nweiz wrote: > On 2016/04/23 09:00:47, Lasse Reichstein Nielsen wrote: > > I do think I'd prefer this class to not be a Set. > > > > I can still have add, remove and contains methods, but probably nothing else. > > It may have a Set getter (Set<Set<E>> get sets) that gives a Set view, but the > > class itself should not be a Set - it's role is to control the GroupSet, not > to > > *be* a Set. That also avoids extending the Set interface. > > Done. > > > Add 'addSet', 'removeSet' to the `GroupSet`. > > It's important that GroupSet *not* have additional methods that aren't in Set. > The fact that a Set exposed by a class is implemented using GroupSet should be > invisible to the user. Why? It's like saying that users expecting an "Iterable" must never get anything that has more methods. With strong mode you can't even access these additional methods accidentally. > > > Keep the set of sets that are used to initialize the `GroupSet`. > > Creating a new SetGroup for every call to [activeTests] is a lot of overhead and > indirection that would be avoided by this class existing. Then cache it. Fwiw I strongly prefer the 'additional methods' approach. https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:28: GroupSet<E> _set; On 2016/04/26 21:23:53, nweiz wrote: > On 2016/04/25 16:11:10, floitsch wrote: > > This should be final. > > Unfortunately it can't be; GroupSet's initializer refers to [this]. True. It was, but isn't anymore. Looking more at it, I think, it is not a good approach to make it possible to change the set after it has been passed to the UnionSet (thus my strong preference to having the additional methods). If we keep the UnionSetController, then in should be in the same library as the UnionSet and just change the UnionSet's private variable. https://codereview.chromium.org/1873373002/diff/140001/lib/src/union_set_cont... File lib/src/union_set_controller.dart (right): https://codereview.chromium.org/1873373002/diff/140001/lib/src/union_set_cont... lib/src/union_set_controller.dart:35: /// disjoint—that is, that they contain no elements in common. This makes Please don't use non-ascii characters. Commonmark uses '--' or '---' for hyphens. If dartdoc doesn't support smart punctuation yet we should rather change that.
https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart File lib/src/group_set.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:29: /// If [disjoint] is `true`, this assumes that all component sets are Disagree. When we use "must" we generally mean that otherwise an error will be raised. In this case there is no error thrown, just inconsistent behavior, and I think it's fine to warn the user that we won't be catching that error for them. We could reduce the text to "should" instead of "must", but it's more than just a recommendation. It really is an error that we just don't catch actively. So, I think the best solution is to say "must" and then say that doing it anyway is not throwing as you would otherwise expect. https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { I generally recommend not extending the well-known interfaces like List, Set, Future, etc. Iterable is special because it is intended as a superclass of collections in general. I wouldn't recommend anybody extending Iterable without good cause - like creating a new kind of collection that makes sense by itself - and not just adding a single method that makes sense in a single setting. An interface of "Set + two more methods" is an unnecessary mental overhead for the user. Either it's a Set or it's SomethingElse, and that SomethingElse should only have the members necessary to fill its role, and not worry about being a Set as well. I very much prefer something that *has* those two methods and *has* a set, to a Set that has those methods as well. https://codereview.chromium.org/1873373002/diff/140001/lib/src/union_set.dart File lib/src/union_set.dart (right): https://codereview.chromium.org/1873373002/diff/140001/lib/src/union_set.dart... lib/src/union_set.dart:16: class UnionSet<E> extends SetMixin<E> with UnmodifiableSetMixin<E> { Extend SetBase instead of SetMixin. They are equivalent, but we have *Base for extension and *Mixin for mixin application. (We should only have one, but until that happens, let's use both as intended).
https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart File lib/src/group_set.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/group_set.dart... lib/src/group_set.dart:29: /// If [disjoint] is `true`, this assumes that all component sets are On 2016/04/27 12:17:53, Lasse Reichstein Nielsen wrote: > Disagree. When we use "must" we generally mean that otherwise an error will be > raised. In this case there is no error thrown, just inconsistent behavior, and I > think it's fine to warn the user that we won't be catching that error for them. > We could reduce the text to "should" instead of "must", but it's more than just > a recommendation. It really is an error that we just don't catch actively. > > So, I think the best solution is to say "must" and then say that doing it anyway > is not throwing as you would otherwise expect. I can live with the comment, so let's just keep it. Personally, I do think that `must` should encompass inconsistent behavior if the rules aren't respected. We shouldn't need to check it. "Must" means bad things will happen if you don't respect the rules. An error is obviously the best bad thing, but inconsistent behavior plainly falls into that category too. https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { On 2016/04/27 12:17:53, Lasse Reichstein Nielsen wrote: > I generally recommend not extending the well-known interfaces like List, Set, > Future, etc. > Iterable is special because it is intended as a superclass of collections in > general. I wouldn't recommend anybody extending Iterable without good cause - > like creating a new kind of collection that makes sense by itself - and not just > adding a single method that makes sense in a single setting. > > An interface of "Set + two more methods" is an unnecessary mental overhead for > the user. Either it's a Set or it's SomethingElse, and that SomethingElse should > only have the members necessary to fill its role, and not worry about being a > Set as well. > I very much prefer something that *has* those two methods and *has* a set, to a > Set that has those methods as well. > I'm not convinced. A UnionSet supports adding complete sets without copying them (using the argument as backing set). As such it *is* a Set and has operations that are just not supported by the base Set. Extending it and adding the additional methods sounds like a perfect use case of subclassing. The GroupSet now just gives access to (currently) private methods of the UnionSet. I don't think that separation is necessary, and that the methods should just be exposed (especially since the new methods are very similar to existing ones like `add` or `addAll`).
https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { The GroupSet is just a Set at the interface level. There doesn't need to be a visible *GroupSet* class at all - that's just an implementation detail and a place to stick the constructors. You might be able to add a set to it, but that too is an implementation detail that is only available to the creator of the GroupSet. If you add methods to that Set then it's a new kind of Set, not just a plain Set. Any place it's passed to can check the type and cast it to get access to the new methods. You'll likely see some use-cases where the GroupSet is wrapped in a SetView to *hide* the extra methods. I prefer a GroupSetController with a set getter and the methods that allow you access to the implementation as necessary. That's what we did in other places, and for the same reasons.
https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { On 2016/04/27 12:47:00, Lasse Reichstein Nielsen wrote: > The GroupSet is just a Set at the interface level. There doesn't need to be a > visible *GroupSet* class at all - that's just an implementation detail and a > place to stick the constructors. > You might be able to add a set to it, but that too is an implementation detail > that is only available to the creator of the GroupSet. > > If you add methods to that Set then it's a new kind of Set, not just a plain > Set. Any place it's passed to can check the type and cast it to get access to > the new methods. You'll likely see some use-cases where the GroupSet is wrapped > in a SetView to *hide* the extra methods. If that's really necessary, yes. That's how it works. It's the same reason we have wrappers that disallow writing operations to Set. > > I prefer a GroupSetController with a set getter and the methods that allow you > access to the implementation as necessary. That's what we did in other places, > and for the same reasons. We do have some cases (Futures and Streams) where we did that separation. But the separated classes have fundamentally different purposes. One is usually the output. The other the input. Here, we just add new methods with similar functionality as the existing ones. `addSet` really is the same as `addAll`, just that it uses the given set as a backing set (without copying the elements). If you are really afraid that users want to modify the set, then you have to wrap it (as with any other Set). I understand that the UnionSet is, by default, an unmodifiable set, but that seems to be more of an implementation issue than a fundamental property of UnionSet. I see the UnionSet much more as an extension to Set than a different class. To me UnionSet is to Set, like EventSink to Sink. Both simply add new methods that add new functionality.
https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { I think our difference is mainly in perspective. I see the GroupSet as a *Set view* of a collection of Sets. It's not interesting in itself, anybody seeing it will just see a Set. You see it as a new kind of collection that allows adding entire sets in constant time. I can't find a good reason for having that kind of collection as a collection (rather than a view), but I'll defer to Natalie on what the actual original intent was - a new kind of collection or a view that looks like a Set.
https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { On 2016/04/27 14:31:15, Lasse Reichstein Nielsen wrote: > I think our difference is mainly in perspective. I see the GroupSet as a *Set > view* of a collection of Sets. It's not interesting in itself, anybody seeing it > will just see a Set. > You see it as a new kind of collection that allows adding entire sets in > constant time. > I can't find a good reason for having that kind of collection as a collection > (rather than a view), but I'll defer to Natalie on what the actual original > intent was - a new kind of collection or a view that looks like a Set. The intent was definitely a view that looks like a set. I'm coming at this from the perspective of exposing information about an existing class, and the controller/external object duality works very well for that use case. https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:28: GroupSet<E> _set; On 2016/04/27 10:56:52, floitsch wrote: > On 2016/04/26 21:23:53, nweiz wrote: > > On 2016/04/25 16:11:10, floitsch wrote: > > > This should be final. > > > > Unfortunately it can't be; GroupSet's initializer refers to [this]. > > True. It was, but isn't anymore. > > Looking more at it, I think, it is not a good approach to make it possible to > change the set after it has been passed to the UnionSet (thus my strong > preference to having the additional methods). > > If we keep the UnionSetController, then in should be in the same library as the > UnionSet and just change the UnionSet's private variable. Allowing a UnionSet to be directly based on an existing Set<Set> seems strictly more expressive than forcing it to only be created using a controller. It also matches the APIs of existing view classes like MapKeySet and MapValueSet. I'm not sure why you wouldn't want to allow it. https://codereview.chromium.org/1873373002/diff/140001/lib/src/union_set_cont... File lib/src/union_set_controller.dart (right): https://codereview.chromium.org/1873373002/diff/140001/lib/src/union_set_cont... lib/src/union_set_controller.dart:35: /// disjoint—that is, that they contain no elements in common. This makes On 2016/04/27 10:56:52, floitsch wrote: > Please don't use non-ascii characters. > Commonmark uses '--' or '---' for hyphens. If dartdoc doesn't support smart > punctuation yet we should rather change that. See my other comment. This rule isn't part of the documentation style guide, and I don't see a strong justification for it.
https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:28: GroupSet<E> _set; Allowing a UnionSet to be directly based on an existing Set<Set> is fine (with me). It's the SetGroup/UnionSetController class I'm concerned about - the one that creates a UnionSet and a way to modify it in one go. That one could be a controller style class (getter for UnionSet, operations that control the underlying set), or it can be just a class that just exposes the UnionSet and the Set<Set>. (I just don't want one to be a property of the other, nor the UnionSet to expose the Set<Set> below it and break the Set interface).
LGTM with SetBase (see Lasse's comment).
Florian: I'm not sure what you mean by "SetBase"; I don't see that in any of Lasse's comments. https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:28: GroupSet<E> _set; On 2016/04/27 21:03:30, Lasse Reichstein Nielsen wrote: > Allowing a UnionSet to be directly based on an existing Set<Set> is fine (with > me). It's the SetGroup/UnionSetController class I'm concerned about - the one > that creates a UnionSet and a way to modify it in one go. > > That one could be a controller style class (getter for UnionSet, operations that > control the underlying set), or it can be just a class that just exposes the > UnionSet and the Set<Set>. (I just don't want one to be a property of the other, > nor the UnionSet to expose the Set<Set> below it and break the Set interface). Right now it's the former: a class that exposes a getter for a UnionSet and methods to control that set.
The SetBase comment is in comment #21. LGTM with that. https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart File lib/src/set_group.dart (right): https://codereview.chromium.org/1873373002/diff/120001/lib/src/set_group.dart... lib/src/set_group.dart:28: GroupSet<E> _set; Yes, so I'm good too :)
Code review changes
https://codereview.chromium.org/1873373002/diff/140001/lib/src/union_set.dart File lib/src/union_set.dart (right): https://codereview.chromium.org/1873373002/diff/140001/lib/src/union_set.dart... lib/src/union_set.dart:16: class UnionSet<E> extends SetMixin<E> with UnmodifiableSetMixin<E> { On 2016/04/27 12:17:53, Lasse Reichstein Nielsen wrote: > Extend SetBase instead of SetMixin. They are equivalent, but we have *Base for > extension and *Mixin for mixin application. > > (We should only have one, but until that happens, let's use both as > intended). Done.
Description was changed from ========== Add GroupSet and SetGroup classes. GroupSet provides a view of the union of a set of sets. SetGroup provides an easy way to manage a GroupSet in a class context. R=lrn@google.com ========== to ========== Add GroupSet and SetGroup classes. GroupSet provides a view of the union of a set of sets. SetGroup provides an easy way to manage a GroupSet in a class context. R=floitsch@google.com, lrn@google.com Committed: https://github.com/dart-lang/collection/commit/e3a6fca00c0c0d732a714941a92b60... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) manually as e3a6fca00c0c0d732a714941a92b6052d211e7bb (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1873373002/diff/140001/lib/src/union_set_cont... File lib/src/union_set_controller.dart (right): https://codereview.chromium.org/1873373002/diff/140001/lib/src/union_set_cont... lib/src/union_set_controller.dart:35: /// disjoint—that is, that they contain no elements in common. This makes On 2016/04/27 20:47:40, nweiz wrote: > On 2016/04/27 10:56:52, floitsch wrote: > > Please don't use non-ascii characters. > > Commonmark uses '--' or '---' for hyphens. If dartdoc doesn't support smart > > punctuation yet we should rather change that. > > See my other comment. This rule isn't part of the documentation style guide, and > I don't see a strong justification for it. Forgot to answer that one: having non-Ascii tokens in source files will make the VM use a non-byte string for the sources. They are twice as big, and, more importantly, will make all use-sites polymorphic. I don't think this requires a style-guide entry. Sometimes, unicode characters are fine, but there is no good reason to use one here.
Message was sent while issue was closed.
On 2016/05/09 13:19:19, floitsch wrote: > Forgot to answer that one: having non-Ascii tokens in source files will make the > VM use a non-byte string for the sources. They are twice as big, and, more > importantly, will make all use-sites polymorphic. > I don't think this requires a style-guide entry. Sometimes, unicode characters > are fine, but there is no good reason to use one here. If that's actually significant, it seems like a non-obvious gotcha that we should fix in the implementation. The same thing should happen if you have a non-ASCII character in a string literal, which is perfectly fine and acceptable. We probably can't see it on most of our benchmarks because they only start counting after the VM has run for a while, but I'd like to see if having a non-ASCII (or non-Latin-1) file really makes a difference. If it doesn't, then I have no problem with Unicode because uses (especially those that actually localize their programs) will hit that anyway. If it does, I think we should fix it. Representing source code as UTF-8 is more efficient than using Dart strings anyway.
Message was sent while issue was closed.
On 2016/05/09 15:52:50, Lasse Reichstein Nielsen wrote: > On 2016/05/09 13:19:19, floitsch wrote: > > > Forgot to answer that one: having non-Ascii tokens in source files will make > the > > VM use a non-byte string for the sources. They are twice as big, and, more > > importantly, will make all use-sites polymorphic. > > I don't think this requires a style-guide entry. Sometimes, unicode characters > > are fine, but there is no good reason to use one here. > > If that's actually significant, it seems like a non-obvious gotcha that we > should fix in the implementation. > The same thing should happen if you have a non-ASCII character in a string > literal, which is perfectly fine and acceptable. > > We probably can't see it on most of our benchmarks because they only start > counting after the VM has run for a while, but I'd like to see if having a > non-ASCII (or non-Latin-1) file really makes a difference. If it doesn't, then I > have no problem with Unicode because uses (especially those that actually > localize their programs) will hit that anyway. If it does, I think we should fix > it. Representing source code as UTF-8 is more efficient than using Dart strings > anyway. As I said: there are good use cases for unicode (and having a string with it is one of them). However, I don't see any good reason here. Even if it's just the fact that the string allocates more memory. Replacing the unicode character with an ascii character doesn't change anything in terms of readability and is just better for the VM. |