Skip to content
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.

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

Implement collections reform #18424

Closed
23 of 24 tasks
alexcrichton opened this issue Oct 29, 2014 · 78 comments
Closed
23 of 24 tasks

Implement collections reform #18424

alexcrichton opened this issue Oct 29, 2014 · 78 comments
Labels
A-collections Area: `std::collection` B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented.

Comments

@alexcrichton
Copy link
Member

Tracking issue for rust-lang/rfcs#235, there are a number of sub-issues associated with this:

Backwards incompatible changes to make:

Backwards compatible changes that will require additional language features:

Backwards compatible changes to make:

@alexcrichton alexcrichton added A-libs B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. labels Oct 29, 2014
@alexcrichton
Copy link
Member Author

My list may not be 100% accurate, so feel free to edit @aturon!

@Gankra
Copy link
Contributor

Gankra commented Oct 29, 2014

@aturon how do you want to coordinate this work?

@gamazeps
Copy link
Contributor

I ca give some help for the (not to hard) bugs :)
@gankro still working on your TODO, but it's a bit more complex than expected at first reading :p

@Gankra
Copy link
Contributor

Gankra commented Oct 29, 2014

I assume it should read "implement deref on strings and vecs"?

@alexcrichton
Copy link
Member Author

Ooops, indeed!

alexcrichton added a commit to alexcrichton/rust that referenced this issue Oct 30, 2014
This commit adds the following impls:

    impl<T> Deref<[T]> for Vec<T>
    impl<T> DerefMut<[T]> for Vec<T>
    impl Deref<str> for String

This commit also removes all duplicated inherent methods from vectors and
strings as implementations will now silently call through to the slice
implementation. Some breakage occurred at std and beneath due to inherent
methods removed in favor of those in the slice traits and std doesn't use its
own prelude,

cc rust-lang#18424
alexcrichton added a commit to alexcrichton/rust that referenced this issue Oct 30, 2014
This commit enables implementations of IndexMut for a number of collections,
including Vec, RingBuf, SmallIntMap, TrieMap, TreeMap, and HashMap. At the same
time this deprecates the `get_mut` methods on vectors in favor of using the
indexing notation.

cc rust-lang#18424
@alexcrichton
Copy link
Member Author

I'll start off by removing the collections traits. @gankro and @gamazeps, do you guys have something you'd like to start on specifically? I figure we can start out by just knocking out all the bullet points above. I can add everyone's name next to what you're working on to make sure we don't conflict. Does that sound ok?

@aturon
Copy link
Member

aturon commented Oct 30, 2014

Thanks for jumping on this @alexcrichton. I'd like to do the Borrow-related stuff, Cow and IntoIterator.

@Gankra
Copy link
Contributor

Gankra commented Oct 30, 2014

@alexcrichton we had agreed (perhaps only informally) to take this opportunity to reorganize the modules. In particular several files define two structures, where one is a convenience wrapper for the other (most of the Sets). I want to split those up like BTree and HashMap do, to make maintenance a bit more friendly. I had hoped to do this first since it will clobber all the history, but that ship has sailed, and there's a few outstanding PRs that would need a nasty rebase. Should we delay that till later, or try to get that in ASAP?

Regardless, I can take on conventions-hell.

@alexcrichton
Copy link
Member Author

Hm, that sounds like a good idea! Why don't we wait for me to remove the collections traits and you to land some conventions stuff, and then we can reorganize, does that sound ok?

I'll assign you to conventions-related tasks

@gamazeps
Copy link
Contributor

@alexcrichton I'm kind of a begginer so I'm not sure I'm able to really evaluate the difficulty of the different parts, if you could assign me to the an easy one I would be very happy :p (I have nothing that I would specifically want to do)

@Gankra
Copy link
Contributor

Gankra commented Oct 30, 2014

@gamazeps Ensuring ____ is implemented should be easy (if it's not already done).

@Gankra
Copy link
Contributor

Gankra commented Oct 30, 2014

Renaming Extendable should also be pretty easy if you're good at search-and-replace.

@Gankra
Copy link
Contributor

Gankra commented Oct 30, 2014

Adding repeat should also be pretty quick.

@alexcrichton
Copy link
Member Author

@gamazeps feel like tackling Extendable => Extend and making sure everyone implements it to start out?

@gamazeps
Copy link
Contributor

Awesome :)

@gamazeps
Copy link
Contributor

@alexcrichton Done :) (compiling for now)
Except for Slice where I don't see where I would implement it :/

@Gankra
Copy link
Contributor

Gankra commented Oct 30, 2014

Slices can't be extended, so that's fine.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Dec 17, 2014
Includes a fix for a small mistake in `fn insert` which is caught by test_insert for len=15, but not len=7.

Part of rust-lang#18424

r? @gankro @csherratt @huonw
@pczarn
Copy link
Contributor

pczarn commented Dec 18, 2014

I'm writing bounded iterators for BTreeMap, too

Gankra added a commit to Gankra/rust that referenced this issue Dec 20, 2014
Part of rust-lang#18424

This commit changes the semantics of `reserve` and `capacity` for Bitv and BitvSet to match conventions. It also introduces the notion of `reserve_index` and `reserve_index_exact` for collections with maximum-index-based capacity semantics.

Deprecates free function constructors in favour of functions on Bitv itself.

Changes `Bitv::pop` to return an Option rather than panicking.

Deprecates and renames several methods in favour of conventions.

Marks several blessed methods as unstable.

This commit also substantially refactors Bitv and BitvSet's implementations. The new implementation is simpler, cleaner, better documented, and more robust against overflows. It also reduces coupling between Bitv and BitvSet. Tests have been seperated into seperate submodules.

Fixes rust-lang#16958

[breaking-change]
bors added a commit that referenced this issue Dec 22, 2014
Part of #18424

This commit changes the semantics of `reserve` and `capacity` for Bitv and BitvSet to match conventions. It also introduces the notion of `reserve_index` and `reserve_index_exact` for collections with maximum-index-based capacity semantics.

Deprecates free function constructors in favour of functions on Bitv itself.

Changes `Bitv::pop` to return an Option rather than panicking.

Deprecates and renames several methods in favour of conventions.

Marks several blessed methods as unstable.

This commit also substantially refactors Bitv and BitvSet's implementations. The new implementation is simpler, cleaner, better documented, and more robust against overflows. It also reduces coupling between Bitv and BitvSet. Tests have been seperated into seperate submodules.

Fixes #16958

[breaking-change]
alexcrichton added a commit to alexcrichton/rust that referenced this issue Dec 22, 2014
Part of rust-lang#18424

This commit changes the semantics of `reserve` and `capacity` for Bitv and BitvSet to match conventions. It also introduces the notion of `reserve_index` and `reserve_index_exact` for collections with maximum-index-based capacity semantics.

Deprecates free function constructors in favour of functions on Bitv itself.

Changes `Bitv::pop` to return an Option rather than panicking.

Deprecates and renames several methods in favour of conventions.

Marks several blessed methods as unstable.

This commit also substantially refactors Bitv and BitvSet's implementations. The new implementation is simpler, cleaner, better documented, and more robust against overflows. It also reduces coupling between Bitv and BitvSet. Tests have been seperated into seperate submodules.

Fixes rust-lang#16958

[breaking-change]
bors added a commit that referenced this issue Dec 22, 2014
Part of #18424

This commit changes the semantics of `reserve` and `capacity` for Bitv and BitvSet to match conventions. It also introduces the notion of `reserve_index` and `reserve_index_exact` for collections with maximum-index-based capacity semantics.

Deprecates free function constructors in favour of functions on Bitv itself.

Changes `Bitv::pop` to return an Option rather than panicking.

Deprecates and renames several methods in favour of conventions.

Marks several blessed methods as unstable.

This commit also substantially refactors Bitv and BitvSet's implementations. The new implementation is simpler, cleaner, better documented, and more robust against overflows. It also reduces coupling between Bitv and BitvSet. Tests have been seperated into seperate submodules.

Fixes #16958

[breaking-change]
@Gankra
Copy link
Contributor

Gankra commented Dec 30, 2014

Now that Bitv is fixed up and the less-well-maintained collections are gone, all the convention issues are fully resolved, I believe. If anyone knows of outstanding API normativity issues, let me know!

Everything else needs lang stuff, or is being brutalized by @pczarn as we speak.

@Gankra
Copy link
Contributor

Gankra commented Dec 30, 2014

(for part 1 issues, that is)

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 6, 2015
Part of collections reform part 1 and 2, rust-lang#18424 and rust-lang#19986

* shrink_to_fit
* swap_back_remove
* swap_front_remove
* truncate
* resize
@kmcallister kmcallister added the A-collections Area: `std::collection` label Jan 16, 2015
bors added a commit that referenced this issue Jan 19, 2015
Part of collections reform v1, #18424
Also, iteration is simplified:
```
before
test btree::map::bench::iter_1000                          ... bench:     17177 ns/iter (+/- 6302)
test btree::map::bench::iter_100000                        ... bench:   1735731 ns/iter (+/- 23908)
test btree::map::bench::iter_20                            ... bench:       386 ns/iter (+/- 148)
after
test btree::map::bench::iter_1000                          ... bench:     15777 ns/iter (+/- 346)
test btree::map::bench::iter_100000                        ... bench:   1602604 ns/iter (+/- 73629)
test btree::map::bench::iter_20                            ... bench:       339 ns/iter (+/- 91)
```
cc @gereeter @cgaebel
r? @gankro
alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 30, 2015
As per [RFC rust-lang#235][rfc], you can now do:

[rfc]: https://github.com/rust-lang/rfcs/blob/master/text/0235-collections-conventions.md#intoiterator-and-iterable

``` rust
let mut v = vec![1];

// iterate over immutable references
for x in &v {
    assert_eq!(x, &1);
}

// iterate over mutable references
for x in &mut v {
    assert_eq!(x, &mut 1);
}

// iterate over values, this consumes `v`
for x in v {
    assert_eq!(x, 1);
}
```

[breaking-change]s

For loops now "consume" (move) the iterator, this breaks iterating over mutable references to iterators, and also breaks multiple iterations over the same iterator:

``` rust
fn foo(mut it: &mut Iter) {  // `Iter` implements `Iterator`
    for x in it { .. }  //~ error: `&mut Iter` doesn't implement Iterator
}

fn bar() {
    for x in it { .. }  //~ note: `it` moved here
    for x in it { .. }  //~ error: `it` has been moved
}
```

Both cases can be fixed using the `by_ref()` adapter to create an iterator from the mutable reference:

``` rust
fn foo(mut it: &mut Iter) {
    for x in it.by_ref() { .. }
}

fn bar() {
    for x in it.by_ref() { .. }
    for x in it { .. }
}
```

This PR also makes iterator non-implicitly copyable, as this was source of subtle bugs in the libraries. You can still use `clone()` to explictly copy the iterator.

Finally, since the for loops are implemented in the frontend and use global paths to `IntoIterator`, `Iterator` and `Option` variants, users of the `core` crate will have to use add an `std` module to the root of their crate to be able to use for loops:

``` rust
#![no_std]

extern crate core;

fn main() {
    for x in 0..10 {}
}

#[doc(hidden)]
mod std {
    // these imports are needed to use for-loops
    pub use core::iter;
    pub use core::option;
}
```

---

r? @nikomatsakis @aturon
cc rust-lang#18424
closes rust-lang#18045
@alexcrichton
Copy link
Member Author

This issue has been growing stale for some time now, and I think that we're basically all done with the implementation work, so closing.

@zommiommy
Copy link
Contributor

From my understanding, this RFC was blocked due to the lack of support for HKT.

The proposed HKT version of the Iterable trait is currently supported in stable:

trait Iterable {
    type A;
    type I<'a>: Iterator<&'a Self::A>;

    fn iter(&self) -> Self::I<'_>;
}

trait IterableMut {
    type A;
    type I<'a>: Iterator<&'a mut Self::A>;

    fn iter_mut(&self) -> Self::I<'_>;
}

so it might be worth re-discussing this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: `std::collection` B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented.
Projects
None yet
Development

No branches or pull requests