Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(83)

Issue 1873373002: Add GroupSet and SetGroup classes. (Closed)

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.

Description

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/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+409 lines, -2 lines) Patch
M CHANGELOG.md View 1 2 3 4 5 6 7 1 chunk +6 lines, -1 line 0 comments Download
M lib/collection.dart View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
A lib/src/union_set.dart View 1 2 3 4 5 6 7 8 1 chunk +88 lines, -0 lines 0 comments Download
A lib/src/union_set_controller.dart View 1 2 3 4 5 6 7 1 chunk +54 lines, -0 lines 0 comments Download
M pubspec.yaml View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
A test/union_set_controller_test.dart View 1 2 3 4 5 6 7 1 chunk +36 lines, -0 lines 0 comments Download
A test/union_set_test.dart View 1 2 3 4 5 6 7 1 chunk +222 lines, -0 lines 0 comments Download

Messages

Total messages: 37 (2 generated)
nweiz
4 years, 8 months ago (2016-04-11 22:08:27 UTC) #1
Lasse Reichstein Nielsen
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#newcode15 lib/src/group_set.dart:15: /// in the earliest inner set is preferred. Worth ...
4 years, 8 months ago (2016-04-12 13:55:22 UTC) #2
nweiz
Code review changes
4 years, 8 months ago (2016-04-12 20:29:15 UTC) #3
nweiz
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#newcode15 lib/src/group_set.dart:15: /// in the earliest inner set is preferred. On ...
4 years, 8 months ago (2016-04-12 20:29:20 UTC) #4
nweiz
Code review changes
4 years, 8 months ago (2016-04-12 23:59:57 UTC) #5
nweiz
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#newcode17 lib/src/set_group.dart:17: /// On 2016/04/12 20:29:20, nweiz wrote: > On 2016/04/12 ...
4 years, 8 months ago (2016-04-13 00:00:57 UTC) #6
Lasse Reichstein Nielsen
On 2016/04/13 00:00:57, nweiz wrote: > After making the change and trying it out, I ...
4 years, 8 months ago (2016-04-13 09:40:15 UTC) #7
nweiz
On 2016/04/13 09:40:15, Lasse Reichstein Nielsen wrote: > On 2016/04/13 00:00:57, nweiz wrote: > > ...
4 years, 8 months ago (2016-04-13 21:13:46 UTC) #8
nweiz
Code review changes
4 years, 8 months ago (2016-04-13 21:30:01 UTC) #9
nweiz
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#newcode9 lib/src/group_set.dart:9: /// A single set that provides a view of ...
4 years, 8 months ago (2016-04-13 21:30:06 UTC) #10
Lasse Reichstein Nielsen
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#newcode9 > ...
4 years, 8 months ago (2016-04-13 23:21:32 UTC) #11
nweiz
Ping? This is blocking some work in test that an internal customer is waiting on.
4 years, 8 months ago (2016-04-22 21:32:49 UTC) #12
Lasse Reichstein Nielsen
+floitsch Ack, sorry. We're still disagreeing on some of the choices, but it's mainly a ...
4 years, 8 months ago (2016-04-23 08:58:07 UTC) #13
Lasse Reichstein Nielsen
4 years, 8 months ago (2016-04-23 08:58:19 UTC) #15
Lasse Reichstein Nielsen
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#newcode25 lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { I do think I'd ...
4 years, 8 months ago (2016-04-23 09:00:47 UTC) #16
floitsch
Please consider changing the names. GroupSet and SetGroup are not used commonly enough to know ...
4 years, 8 months ago (2016-04-25 16:11:10 UTC) #17
nweiz
Code review changes
4 years, 7 months ago (2016-04-26 21:23:47 UTC) #18
nweiz
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#newcode17 lib/src/group_set.dart:17: class GroupSet<E> extends SetMixin<E> with UnmodifiableSetMixin<E> { On 2016/04/25 ...
4 years, 7 months ago (2016-04-26 21:23:53 UTC) #19
floitsch
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#newcode29 lib/src/group_set.dart:29: /// If [disjoint] is `true`, this assumes ...
4 years, 7 months ago (2016-04-27 10:56:53 UTC) #20
Lasse Reichstein Nielsen
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#newcode29 lib/src/group_set.dart:29: /// If [disjoint] is `true`, this assumes that all ...
4 years, 7 months ago (2016-04-27 12:17:53 UTC) #21
floitsch
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#newcode29 lib/src/group_set.dart:29: /// If [disjoint] is `true`, this assumes that all ...
4 years, 7 months ago (2016-04-27 12:31:21 UTC) #22
Lasse Reichstein Nielsen
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#newcode25 lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { The GroupSet is just ...
4 years, 7 months ago (2016-04-27 12:47:00 UTC) #23
floitsch
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#newcode25 lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { On 2016/04/27 12:47:00, Lasse ...
4 years, 7 months ago (2016-04-27 13:08:47 UTC) #24
Lasse Reichstein Nielsen
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#newcode25 lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { I think our difference ...
4 years, 7 months ago (2016-04-27 14:31:15 UTC) #25
nweiz
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#newcode25 lib/src/set_group.dart:25: class SetGroup<E> extends DelegatingSet<Set<E>> { On 2016/04/27 14:31:15, Lasse ...
4 years, 7 months ago (2016-04-27 20:47:40 UTC) #26
Lasse Reichstein Nielsen
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#newcode28 lib/src/set_group.dart:28: GroupSet<E> _set; Allowing a UnionSet to be directly based ...
4 years, 7 months ago (2016-04-27 21:03:30 UTC) #27
floitsch
LGTM with SetBase (see Lasse's comment).
4 years, 7 months ago (2016-04-29 16:22:06 UTC) #28
nweiz
Florian: I'm not sure what you mean by "SetBase"; I don't see that in any ...
4 years, 7 months ago (2016-05-03 01:05:37 UTC) #29
Lasse Reichstein Nielsen
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#newcode28 ...
4 years, 7 months ago (2016-05-03 08:27:13 UTC) #30
nweiz
Code review changes
4 years, 7 months ago (2016-05-03 17:54:23 UTC) #31
nweiz
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#newcode16 lib/src/union_set.dart:16: class UnionSet<E> extends SetMixin<E> with UnmodifiableSetMixin<E> { On 2016/04/27 ...
4 years, 7 months ago (2016-05-03 17:54:31 UTC) #32
nweiz
Committed patchset #9 (id:160001) manually as e3a6fca00c0c0d732a714941a92b6052d211e7bb (presubmit successful).
4 years, 7 months ago (2016-05-03 17:55:01 UTC) #34
floitsch
https://codereview.chromium.org/1873373002/diff/140001/lib/src/union_set_controller.dart File lib/src/union_set_controller.dart (right): https://codereview.chromium.org/1873373002/diff/140001/lib/src/union_set_controller.dart#newcode35 lib/src/union_set_controller.dart:35: /// disjoint—that is, that they contain no elements in ...
4 years, 7 months ago (2016-05-09 13:19:19 UTC) #35
Lasse Reichstein Nielsen
On 2016/05/09 13:19:19, floitsch wrote: > Forgot to answer that one: having non-Ascii tokens in ...
4 years, 7 months ago (2016-05-09 15:52:50 UTC) #36
floitsch
4 years, 7 months ago (2016-05-09 15:59:30 UTC) #37
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.

Powered by Google App Engine
This is Rietveld 408576698