-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Conversation
Also cc @RalfJung since there's so many, sigh, risky node pointers. |
The job Click to expand the log.
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 |
039bdba
to
64714f8
Compare
The job Click to expand the log.
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 |
64714f8
to
2f938fd
Compare
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. |
There was a problem hiding this 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.
@Amanieu thank you for the review! I also find an inconsistency when implementing. |
I feel that this is a mistake in the design of |
beaf478
to
6d4960c
Compare
Updated the implementation according to the review comments. |
6d4960c
to
d59e9b4
Compare
There was a problem hiding this 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.
There was a problem hiding this 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
f14f3e9
to
a268ebf
Compare
a268ebf
to
d2c509a
Compare
Updated the implementation according to discussions in rust-lang/rfcs#2847 |
1458553
to
06b9a73
Compare
Looks good! The documentation will need more work (especially examples) before this can be stabilized, but the implementation is fine. @bors r+ |
📌 Commit 06b9a73 has been approved by |
Implement Cursor for linked lists. (RFC 2570). cc. rust-lang#58533 cc. @Gankra r? @Amanieu
Implement Cursor for linked lists. (RFC 2570). cc. rust-lang#58533 cc. @Gankra r? @Amanieu
Implement Cursor for linked lists. (RFC 2570). cc. rust-lang#58533 cc. @Gankra r? @Amanieu
Implement Cursor for linked lists. (RFC 2570). cc. rust-lang#58533 cc. @Gankra r? @Amanieu
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
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. |
|
cc. #58533 cc. @Gankra
r? @Amanieu