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 Cursor for linked lists. (RFC 2570). #68123

Merged
merged 4 commits into from
Jan 15, 2020

Conversation

crlf0710
Copy link
Member

@crlf0710 crlf0710 commented Jan 11, 2020

cc. #58533 cc. @Gankra

r? @Amanieu

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 11, 2020
@crlf0710
Copy link
Member Author

Also cc @RalfJung since there's so many, sigh, risky node pointers.

@rust-highfive
Copy link
Collaborator

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-01-11T08:58:14.2752251Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-11T08:58:14.2836394Z ##[command]git config gc.auto 0
2020-01-11T08:58:14.2921207Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-11T08:58:14.2998595Z ##[command]git config --get-all http.proxy
2020-01-11T08:58:14.3155453Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/68123/merge:refs/remotes/pull/68123/merge
---
2020-01-11T09:03:12.0502648Z     Checking alloc v0.0.0 (/checkout/src/liballoc)
2020-01-11T09:03:12.3207028Z     Checking rustc-demangle v0.1.16
2020-01-11T09:03:12.6393541Z     Checking panic_abort v0.0.0 (/checkout/src/libpanic_abort)
2020-01-11T09:03:12.8181543Z     Checking backtrace v0.3.40
2020-01-11T09:03:14.3225518Z error: type does not implement `fmt::Debug`; consider adding `#[derive(Debug)]` or a manual implementation
2020-01-11T09:03:14.3229983Z      |
2020-01-11T09:03:14.3229983Z      |
2020-01-11T09:03:14.3230591Z 1069 | / pub struct Cursor<'a, T: 'a> {
2020-01-11T09:03:14.3231149Z 1070 | |     index: usize,
2020-01-11T09:03:14.3231692Z 1071 | |     current: Option<NonNull<Node<T>>>,
2020-01-11T09:03:14.3232206Z 1072 | |     list: &'a LinkedList<T>,
2020-01-11T09:03:14.3233180Z      | |_^
2020-01-11T09:03:14.3233627Z      |
2020-01-11T09:03:14.3234658Z      = note: `-D missing-debug-implementations` implied by `-D warnings`
2020-01-11T09:03:14.3235027Z 
2020-01-11T09:03:14.3235027Z 
2020-01-11T09:03:14.3235664Z error: type does not implement `fmt::Debug`; consider adding `#[derive(Debug)]` or a manual implementation
2020-01-11T09:03:14.3236725Z      |
2020-01-11T09:03:14.3236725Z      |
2020-01-11T09:03:14.3237292Z 1088 | / pub struct CursorMut<'a, T: 'a> {
2020-01-11T09:03:14.3238051Z 1089 | |     index: usize,
2020-01-11T09:03:14.3238618Z 1090 | |     current: Option<NonNull<Node<T>>>,
2020-01-11T09:03:14.3239512Z 1091 | |     list: &'a mut LinkedList<T>,
2020-01-11T09:03:14.3240877Z      | |_^
2020-01-11T09:03:14.3241079Z 
2020-01-11T09:03:14.3742229Z error: aborting due to 2 previous errors
2020-01-11T09:03:14.3742668Z 
---
2020-01-11T09:03:14.3955112Z   local time: Sat Jan 11 09:03:14 UTC 2020
2020-01-11T09:03:14.6799054Z   network time: Sat, 11 Jan 2020 09:03:14 GMT
2020-01-11T09:03:14.6800077Z == end clock drift check ==
2020-01-11T09:03:20.8361279Z 
2020-01-11T09:03:20.8481110Z ##[error]Bash exited with code '1'.
2020-01-11T09:03:20.8514707Z ##[section]Starting: Checkout
2020-01-11T09:03:20.8516420Z ==============================================================================
2020-01-11T09:03:20.8516480Z Task         : Get sources
2020-01-11T09:03:20.8516548Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-01-11T09:13:53.6211453Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-11T09:13:53.6223710Z ##[command]git config gc.auto 0
2020-01-11T09:13:53.6227901Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-11T09:13:53.6231025Z ##[command]git config --get-all http.proxy
2020-01-11T09:13:53.6233948Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/68123/merge:refs/remotes/pull/68123/merge
---
2020-01-11T09:18:37.3766405Z     Checking alloc v0.0.0 (/checkout/src/liballoc)
2020-01-11T09:18:37.6567906Z     Checking rustc-demangle v0.1.16
2020-01-11T09:18:37.9798651Z     Checking panic_abort v0.0.0 (/checkout/src/libpanic_abort)
2020-01-11T09:18:38.1471625Z     Checking backtrace v0.3.40
2020-01-11T09:18:39.6720194Z error: type does not implement `fmt::Debug`; consider adding `#[derive(Debug)]` or a manual implementation
2020-01-11T09:18:39.6721989Z      |
2020-01-11T09:18:39.6721989Z      |
2020-01-11T09:18:39.6722334Z 1070 | / pub struct Cursor<'a, T: 'a> {
2020-01-11T09:18:39.6731542Z 1071 | |     index: usize,
2020-01-11T09:18:39.6732892Z 1072 | |     current: Option<NonNull<Node<T>>>,
2020-01-11T09:18:39.6733509Z 1073 | |     list: &'a LinkedList<T>,
2020-01-11T09:18:39.6734580Z      | |_^
2020-01-11T09:18:39.6735145Z      |
2020-01-11T09:18:39.6735814Z      = note: `-D missing-debug-implementations` implied by `-D warnings`
2020-01-11T09:18:39.6736022Z 
2020-01-11T09:18:39.6736022Z 
2020-01-11T09:18:39.6736543Z error: type does not implement `fmt::Debug`; consider adding `#[derive(Debug)]` or a manual implementation
2020-01-11T09:18:39.6737467Z      |
2020-01-11T09:18:39.6737467Z      |
2020-01-11T09:18:39.6738023Z 1089 | / pub struct CursorMut<'a, T: 'a> {
2020-01-11T09:18:39.6738541Z 1090 | |     index: usize,
2020-01-11T09:18:39.6739411Z 1091 | |     current: Option<NonNull<Node<T>>>,
2020-01-11T09:18:39.6740103Z 1092 | |     list: &'a mut LinkedList<T>,
2020-01-11T09:18:39.6741265Z      | |_^
2020-01-11T09:18:39.6741443Z 
2020-01-11T09:18:39.7230934Z error: aborting due to 2 previous errors
2020-01-11T09:18:39.7231050Z 
---
2020-01-11T09:18:39.7473865Z   local time: Sat Jan 11 09:18:39 UTC 2020
2020-01-11T09:18:39.9011440Z   network time: Sat, 11 Jan 2020 09:18:39 GMT
2020-01-11T09:18:39.9011518Z == end clock drift check ==
2020-01-11T09:18:40.8006471Z 
2020-01-11T09:18:40.8112786Z ##[error]Bash exited with code '1'.
2020-01-11T09:18:40.8169056Z ##[section]Starting: Checkout
2020-01-11T09:18:40.8170846Z ==============================================================================
2020-01-11T09:18:40.8170897Z Task         : Get sources
2020-01-11T09:18:40.8170938Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@RalfJung
Copy link
Member

I'm afraid I am entirely unfamiliar with this code and will not be able to help much with review here.

You could try running the tests in Miri using the setup at https://github.com/RalfJung/miri-test-libstd; let me know if you need any assistance with that.

Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

In the RFC thread, I suggested some better method names. Unfortunately this didn't seem to make it into the RFC text itself. Can you rename your code to use these new names?

I've added some initial comments on the implementation, but I'll probably have to do another round of review after you fix those. In particular, the doc comments are pretty bare and are missing a lot of information regarding the exact behavior of the methods in every situation.

src/liballoc/collections/linked_list.rs Outdated Show resolved Hide resolved
src/liballoc/collections/linked_list.rs Show resolved Hide resolved
src/liballoc/collections/linked_list.rs Outdated Show resolved Hide resolved
src/liballoc/collections/linked_list.rs Outdated Show resolved Hide resolved
src/liballoc/collections/linked_list.rs Outdated Show resolved Hide resolved
src/liballoc/collections/linked_list.rs Show resolved Hide resolved
src/liballoc/collections/linked_list.rs Outdated Show resolved Hide resolved
src/liballoc/collections/linked_list.rs Show resolved Hide resolved
@crlf0710
Copy link
Member Author

@Amanieu thank you for the review! I also find an inconsistency when implementing. LinkedList::append takes the other list by mutable reference, while the insert_list(will become splice_after) takes the other list by value. Should i change it to take a mutable reference or leave it as is?

@Amanieu
Copy link
Member

Amanieu commented Jan 11, 2020

I feel that this is a mistake in the design of append. It really should have taken the list by value.

@jonas-schievink jonas-schievink added A-collections Area: `std::collection` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 11, 2020
@crlf0710 crlf0710 force-pushed the linked_list_cursor branch 2 times, most recently from beaf478 to 6d4960c Compare January 11, 2020 17:41
@crlf0710
Copy link
Member Author

Updated the implementation according to the review comments.

Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

Nice work!

For the documentation, feel from to copy the doc comments from my intrusive-collections crate.

src/liballoc/collections/linked_list.rs Outdated Show resolved Hide resolved
src/liballoc/collections/linked_list.rs Show resolved Hide resolved
src/liballoc/collections/linked_list.rs Outdated Show resolved Hide resolved
src/liballoc/collections/linked_list.rs Outdated Show resolved Hide resolved
src/liballoc/collections/linked_list.rs Outdated Show resolved Hide resolved
Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

One additional feature you might want to add is an index method which returns Option<usize>. It's free since we need to track the index for splitting anyways. The Debug implementation should be changed to use Option<usize> for the index so that it shows None when pointing to the ghost element.

cc @LukasKalbertodt to update the RFC with this suggestion

src/liballoc/collections/linked_list.rs Show resolved Hide resolved
src/liballoc/collections/linked_list.rs Show resolved Hide resolved
@crlf0710 crlf0710 force-pushed the linked_list_cursor branch 2 times, most recently from f14f3e9 to a268ebf Compare January 12, 2020 10:14
@crlf0710
Copy link
Member Author

Updated the implementation according to discussions in rust-lang/rfcs#2847

@Amanieu
Copy link
Member

Amanieu commented Jan 14, 2020

Looks good! The documentation will need more work (especially examples) before this can be stabilized, but the implementation is fine.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 14, 2020

📌 Commit 06b9a73 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 14, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 15, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 15, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 15, 2020
bors added a commit that referenced this pull request Jan 15, 2020
Rollup of 6 pull requests

Successful merges:

 - #68123 (Implement Cursor for linked lists. (RFC 2570).)
 - #68212 (Suggest to shorten temporary lifetime during method call inside generator)
 - #68232 (Optimize size/speed of Unicode datasets)
 - #68236 (Add some regression tests)
 - #68237 (Account for `Path`s in `is_suggestable_infer_ty`)
 - #68252 (remove redundant clones, found by clippy)

Failed merges:

r? @ghost
@bors bors merged commit 06b9a73 into rust-lang:master Jan 15, 2020
@crlf0710 crlf0710 deleted the linked_list_cursor branch January 16, 2020 00:37
@zbraniecki
Copy link

Has it been considered to add ability to insert/remove elements at cursor? That seems like a core feature to avoid costly inserts and removals at index calculated on the list instance.

@Amanieu
Copy link
Member

Amanieu commented Jan 7, 2021

CursorMut has insert_before, insert_after, remove_current and a bunch of other methods for 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` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants