Skip to content

Completely rewrote the list lib #11453

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

Closed
wants to merge 5 commits into from
Closed

Completely rewrote the list lib #11453

wants to merge 5 commits into from

Conversation

TisButMe
Copy link

Better:
-> No use of the @pointers anymore
-> Has methods instead of functions
-> Fully tested and documented

New:
-> Provides copying/referencing/moving iterators, which implement the Iterator trait (which wasn't the case before. This allows the use of the all "free" methods which come with this trait, as well as the 'for in' syntax.
-> Implements FromIterator trait, which allows this collection to play nice with collect() and similar.
-> Has a prepend method. (Furthermore, it is efficiently implemented: no copying)
-> Has an insert method
-> Has methods to access an element by its index (either copying -at() or []-, referencing -at_ref()- or mutably referencing -at_ref_mut()-)
-> Generally tries to play with the standard Traits instead of re-inventing the wheel.

Good:
-> No use at all of unsafe code, or unsafe pointers.

TODO:
-> At the moment, the FromIterator implementation is naive, and uses repeated append()s which will first go through the whole list. I tried keeping a ref to the last Cell (which should be easy since append() returns a &mut to the last cell), but lifetime management defeated me...
-> Add a MutRef iterator. I started working on this, but can't figure out where I'm going wrong or how to side-step the issue (see http://stackoverflow.com/questions/20993909/borrow-vs-mutable-borrow-strange-failure-in-lifetimes)

Better:
-> No use of the @pointers anymore
-> Has methods instead of functions
-> Fully tested and documented

New:
-> Provides copying/referencing/moving iterators, which implement the Iterator trait (which wasn't the case before. This allows the use of the all "free" methods which come with this trait, as well as the 'for in' syntax.
-> Implements FromIterator trait, which allows this collection to play nice with collect() and similar.
-> Has a prepend method. (Furthermore, it is efficiently implemented: no copying)
-> Has an insert method
-> Has methods to access an element by its index (either copying -at() or []-, referencing -at_ref()- or mutably referencing -at_ref_mut()-)
-> Generally tries to play with the standard Traits instead of re-inventing the wheel.

Good:
-> No use at all of unsafe code, or unsafe pointers.

TODO:
-> At the moment, the FromIterator implementation is naive, and uses repeated append()s which will first go through the whole list. I tried keeping a ref to the last Cell (which should be easy since append() returns a &mut to the last cell), but lifetime management defeated me... 
-> Add a MutRef iterator. I started working on this, but can't figure out where I'm going wrong or how to side-step the issue (see http://stackoverflow.com/questions/20993909/borrow-vs-mutable-borrow-strange-failure-in-lifetimes)
You can therefore

*/
pub fn mov_iter(mut ~self) -> LinkedListMoveIterator<T> {
Copy link
Member

Choose a reason for hiding this comment

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

this should be called move_iter

@huonw
Copy link
Member

huonw commented Jan 10, 2014

FWIW, I don't think an owned singly linked list is particularly useful: using shared ownership (like @ used to allow, and Rc can allow here) lets structure be shared cheaply between instances, e.g. if you have a list Cons(0, Cons(1, Cons(2, Nil))) and you wish to change the head element to 10, there's no need to clone the whole list (as would be necessary with ~), just reuse the Cons(1, Cons(2, Nil)) part to create Cons(10, Cons(1, ...)).

@emberian
Copy link
Member

I was also going to comment on that; I haven't finished a review, and this is certainly nice code (good job!) but I can't think of a usecase.

//! A standard, garbage-collected linked list.
/*!
* This is a straight-forward implementation of a linked-list.
* The implementation in extra uses @pointers, and doesn't use the iterator trait,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think sentence is sensible to put in the module documentation (this appears in the documentation generated for libextra). This code is in libextra right now, and the implementation you are comparing against is deleted by this commit.

Also, I think the prevailing trend is to use //! doc comments, e.g.

//! This is a ...
//! ...
//! ...

@TisButMe
Copy link
Author

You shouldn't have to clone anything, use at_mut_ref(0), and you'll get a mutable ref to the head, see the at_mut_ref test for a demo. As far as I know (I'm still learning rust), this call doesn't copy anything, and allows modification in place.

@huonw
Copy link
Member

huonw commented Jan 10, 2014

I meant so that the programmer is left with "two" lists: one 0 1 2 and the other 10 1 2. The fact that they could share tails is mostly an implementation detail (and if this list were restricted to only containing Freeze types, then it would be entirely an implementation detail).

An owned singly-linked list like this is isomorphic to a stack implemented using a vector, and the vector form is far more efficient:

  • contiguous in memory; nicer for caches
  • O(1) length & indexing
  • very fast iteration

Converting this code to use Rc<> rather than ~ would be better from my perspective. (Although the moving and mutable iterators would have to be removed too.)

@TisButMe
Copy link
Author

Ok, got it.

I kind of wanted to avoid Rc<> since this was written as a way to get more familiar with Rust's pointers, but maybe the lack of RC is better for lower-level code or embedded stuff ? I haven't looked into Rc much, but I thought it caused higher reliance on the runtime ?

This kind of singly linked list still has faster insertions than plain old vector right ?

…opying iterator as LinkedListCopyIterator, created by calling copy_iter() on a list
@thestinger
Copy link
Contributor

@TisButMe: No, a vector has significantly faster insertion than a linked list. When the capacity is full, it doubles the size. I don't think a non-intrusive singly-linked list has a use case, and I think it should just be dropped from the standard library.

@TisButMe
Copy link
Author

Alright :)

Please close the PR, then.

@pcwalton pcwalton closed this Jan 10, 2014
@pcwalton
Copy link
Contributor

Thanks for the PR! All contribution is very valued :)

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 1, 2023
…lyxyas

New lint `clippy::join_absolute_paths`

Hey `@ofeeg,` this PR is a copy of all the changes you did in rust-lang/rust-clippy@47171d3 from PR rust-lang/rust-clippy#11208. I wasn't sure how to fix the git history. Hope you're okay with me figuring this out in this separate PR

---

changelog: New lint [`join_absolute_paths`]
[rust-lang#11453](rust-lang/rust-clippy#11453)

Closes: rust-lang/rust-clippy#10655

r? `@Centri3` Since you also gave feedback on the other PR. I hope that I copied everything correctly, but a third pair of eyes would be appreciated :D
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants