General clean-up and tweaks. #157
Conversation
Fixes some null-safety issues, usees some newer features (const classes can use mixins now), and replaces unnecessary `late` with a dual constructor. Fixes the `CombinedList.iterator` having potentially quadratic complexity since the default list iterator does repeated indexing. Ensure that interfaces returning an `Iterable` does not return a `Set` instead.
| class CombinedIterator<T> implements Iterator<T> { | ||
| /// The iterators that this combines, or `null` if done iterating. | ||
| /// | ||
| /// Because this comes from a call to [Iterable.map], it's lazy and will |
There was a problem hiding this comment.
I worry this comment will become stale now that it is no longer an implementation detail of a single library. Since it should still be private to the package this might not be a huge deal, but we should consider moving the comment to the constructor to be something like "iterators should not eagerly instantiate iterators which have not been reached".
There was a problem hiding this comment.
It is still an implementation detail, the class is not exported by the package.
I considered combining the combined list/iterable iterators in a single library, and making the helper private. Would you prefer that?
| // Unfortunately, we can't use UnmodifiableSetMixin here, since const classes | ||
| // can't use mixins. | ||
| /// An unmodifiable, empty set that can have a const constructor. | ||
| /// An unmodifiable, empty set which can be constant. | ||
| class EmptyUnmodifiableSet<E> extends IterableBase<E> |
There was a problem hiding this comment.
[out of scope for this PR] Is this class buying value over const <E>{}? Should we consider deprecating it?
There was a problem hiding this comment.
Probably not worth it any more. It's a valid superclass, but I can't see for what.
Yes, we should consider deprecating it.
lib/src/union_set_controller.dart
Outdated
|
|
||
| /// The sets whose union is exposed through [set]. | ||
| final _sets = <Set<E>>{}; | ||
| final _sets; |
There was a problem hiding this comment.
Can we add a type?
| final _sets; | |
| final Set<Set<E>> _sets; |
There was a problem hiding this comment.
Doh, yes. That's why I prefer always writing the types, they keep working when you remove the initializer!
* General clean-up and tweaks. Fixes some null-safety issues, usees some newer features (const classes can use mixins now), and replaces unnecessary `late` with a dual constructor. Fixes the `CombinedList.iterator` having potentially quadratic complexity since the default list iterator does repeated indexing. Ensure that interfaces returning an `Iterable` does not return a `Set` instead.
Fixes some null-safety issues, usees some newer features (const classes can use mixins now),
and replaces unnecessary
latewith a dual constructor.Fixes the
CombinedList.iteratorhaving potentially quadratic complexitysince the default list iterator does repeated indexing.
Ensure that interfaces returning an
Iterabledoes not return aSetinstead.