Skip to content

RFC: Collections reform #235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alexcrichton merged 7 commits into from
Oct 29, 2014
Merged

RFC: Collections reform #235

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5c31395
RFC: Collections reform
aturon Sep 5, 2014
4034f4a
Added drawbacks
aturon Sep 11, 2014
8ecd360
A few minor fixes
aturon Sep 11, 2014
4490dc7
Added alternative discussion for `for`
aturon Sep 11, 2014
d4e9e71
Fix bad references to self
aturon Sep 11, 2014
7eadf34
Substantially revised Borrow trait, add range proposal
aturon Sep 18, 2014
3825c86
Update with minor clarifications and adjustments
aturon Oct 27, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Update with minor clarifications and adjustments
  • Loading branch information
@aturon
aturon committed Oct 27, 2014
commit 3825c868bf39741f21f05d5d8c2c65f27637f599
34 changes: 19 additions & 15 deletions active/0000-collections-conventions.md → text/0000-collection-conventions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,6 +975,7 @@ Operation | Collections
`fn insert(&mut self, K::Owned) -> bool` | `HashSet`, `TreeSet`, `TrieSet`, `BitvSet`
`fn insert(&mut self, K::Owned, V) -> Option<V>` | `HashMap`, `TreeMap`, `TrieMap`, `SmallIntMap`
`fn append(&mut self, Self)` | `DList`
`fn prepend(&mut self, Self)` | `DList`

There are a few changes here from the current state of affairs:

Expand Down Expand Up @@ -1238,13 +1239,13 @@ fn difference<'a>(&'a self, other: &'a Self) -> I;
fn symmetric_difference<'a>(&'a self, other: &'a Self) -> I;
```

where the `I` type is an iterator over keys that varies by concrete set. Working
with these iterators avoids materializing intermediate sets when they're not
needed; the `collect` method can be used to create sets when they are.

To clarify the API, this RFC proposes renaming the methods to `iter_or`,
`iter_and`, `iter_sub`, and `iter_xor` respectively. These names emphasize the
fact that the methods return iterators, which may be surprising.
where the `I` type is an iterator over keys that varies by concrete
set. Working with these iterators avoids materializing intermediate
sets when they're not needed; the `collect` method can be used to
create sets when they are. This RFC proposes to keep these names
intact, following the
[RFC](https://github.com/rust-lang/rfcs/pull/344) on iterator
conventions.

Sets should also implement the `BitOr`, `BitAnd`, `BitXor` and `Sub` traits from
Copy link
Contributor

@huonw huonw Sep 20, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They could return iterators, and then these iterators could implement them same operators (if they are Copy or efficiently Cloneable), meaning (a | b) & c would return an iterator representing the union of a and b all intersected with c. This may be even more suprising, but seems slick.

Copy link
Contributor

@Gankra Gankra Sep 20, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this functionality could actually be provided generically on iterators that assert that their contents are sorted. Then we don't need to impl this stuff on each Set impl, we just make their iterators impl the (empty) SortedIterator trait. Then it's just (a.iter() | b.iter()) & c.iter(). Less slick, but much more composable and less of a double-dispatch hell, I think.

Copy link
Contributor Author

@aturon aturon Sep 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered moving the various set operations to the iterator level. Indeed, this is how they're implemented in some -- but not all -- cases. So it's partly a question of whether we want to be able to provide specialized implementations (although trait specialization, if it happens, may eventually help there.)

In any case, it might be worth doing some tests to see whether we can get away with generic combinators, as that would be in line with the overall design goals of this RFC.

`std::ops`, allowing overloaded notation `|`, `&`, `|^` and `-` to be used with
Expand Down Expand Up @@ -1487,13 +1488,15 @@ v.push_all_move(some_vec);
v.extend(some_vec);
```

However, currently the `push_all` and `push_all_move` methods can rely on the
*exact* size of the container being pushed, in order to elide bounds checks. We
do not currently have a way to "trust" methods like `len` on iterators to elide
bounds checks. A separate RFC will introduce the notion of a "trusted" method
which should support such optimization and allow us to deprecate the `push_all`
and `push_all_move` variants. (This is unlikely to happen before 1.0, so the
methods will probably still be included with "experimental" status.)
However, currently the `push_all` and `push_all_move` methods can rely
on the *exact* size of the container being pushed, in order to elide
bounds checks. We do not currently have a way to "trust" methods like
`len` on iterators to elide bounds checks. A separate RFC will
introduce the notion of a "trusted" method which should support such
optimization and allow us to deprecate the `push_all` and
`push_all_move` variants. (This is unlikely to happen before 1.0, so
the methods will probably still be included with "experimental"
status, and likely with different names.)

# Alternatives

Expand Down Expand Up @@ -1767,4 +1770,5 @@ Using the `Borrow` trait, it might be possible to safely add a coercion for auto
For sized types, this coercion is *forced* to be trivial, so the only time it
would involve running user code is for unsized values.

A general story about such coercions will be left to a follow-up RFC.
A general story about such coercions will be left to a
[follow-up RFC](https://github.com/rust-lang/rfcs/pull/241).