Skip to content

Add linked list cursor end methods #86714

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

Merged
merged 3 commits into from
Jul 2, 2021

Conversation

iwahbe
Copy link
Contributor

@iwahbe iwahbe commented Jun 29, 2021

I add several methods to LinkedList::CursorMut and LinkedList::Cursor. These methods allow you to access/manipulate the ends of a list via the cursor. This is especially helpful when scanning through a list and reordering. For example:

let mut c = ll.back_cursor_mut();
let mut moves = 10;
while c.current().map(|x| x > 5).unwrap_or(false) {
    let n = c.remove_current();
    c.push_front(n);
    if moves > 0 { break; } else { moves -= 1; }
}

I encountered this problem working on my bachelors thesis doing graph index manipulation.

While this problem can be avoided by splicing, it is awkward. I asked about the problem here and it was suggested I write a PR.

All methods added consist of

Cursor::front(&self) -> Option<&T>;
Cursor::back(&self) -> Option<&T>;
CursorMut::front(&self) -> Option<&T>;
CursorMut::back(&self) -> Option<&T>;
CursorMut::front_mut(&mut self) -> Option<&mut T>;
CursorMut::back_mut(&mut self) -> Option<&mut T>;
CursorMut::push_front(&mut self, elt: T);
CursorMut::push_back(&mut self, elt: T);
CursorMut::pop_front(&mut self) -> Option<T>;
CursorMut::pop_back(&mut self) -> Option<T>;

Design decisions:

I tried to remain as consistent as possible with what was already present for linked lists.
The methods front, front_mut, back and back_mut are identical to their LinkedList equivalents.

I tried to make the pop_front and pop_back methods work the same way (vis a vis the "ghost" node) as remove_current. I thought this was the closest analog.

push_front and push_back do not change the "current" node, even if it is the "ghost" node. I thought it was most intuitive to say that if you add to the list, current will never change.

Any feedback would be welcome 😄

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 29, 2021
@Mark-Simulacrum
Copy link
Member

r? @Amanieu

/// Provides a reference to the front element of the cursor's parent list,
/// or None if the list is empty.
#[unstable(feature = "linked_list_cursors", issue = "58533")]
pub fn front(&self) -> Option<&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 return &'a T. The difference is subtle but effectively allows the returned reference to outlive the Cursor. Note that this is only valid for Cursor and not CursorMut.

// Safety: We know that `push_back` does not change the position in
// memory of other nodes. This ensures that `self.current` remains
// valid.
self.list.push_back(elt);
Copy link
Member

Choose a reason for hiding this comment

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

You need to update index if the cursor is pointing to the ghost element. See the code in insert_after.

}
// We always need to change the index since `head` comes before any
// other element.
self.index.checked_sub(1).unwrap_or(0);
Copy link
Member

Choose a reason for hiding this comment

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

Index subtraction can be moved to the else case, which should eliminate the need for checked_sub.

/// Removes the last element from the cursor's parent list and returns it,
/// or None if the list is empty. The element the cursor points to remains
/// unchanged, unless it was pointing to the back element. In that case, it
/// points to the new back element.
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit surprising: everywhere else we move to the next element when the current cursor position is removed. It would make more sense to move to the ghost element in this case.

@iwahbe iwahbe requested a review from Amanieu July 1, 2021 19:14
@iwahbe
Copy link
Contributor Author

iwahbe commented Jul 1, 2021

All of the comments made sense, though the first one took me a little while to understand why it didn't apply to CursorMut. They should be addressed in c4ad273.

@Amanieu
Copy link
Member

Amanieu commented Jul 1, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 1, 2021

📌 Commit c4ad273 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 Jul 1, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 2, 2021
…laumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#85749 (Revert "Don't load all extern crates unconditionally")
 - rust-lang#86714 (Add linked list cursor end methods)
 - rust-lang#86737 (Document rustfmt on nightly-rustc)
 - rust-lang#86776 (Skip layout query when computing integer type size during mangling)
 - rust-lang#86797 (Stabilize `Bound::cloned()`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ccdbda6 into rust-lang:master Jul 2, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 2, 2021
@iwahbe iwahbe deleted the add-linked-list-cursor-end-methods branch December 29, 2021 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants