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.

Sign up for GitHub

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

Jump to bottom

Fix linked list cursor names #2847

Merged
merged 4 commits into from
Jan 14, 2020
Merged

Fix linked list cursor names #2847

merged 4 commits into from
Jan 14, 2020

Conversation

LukasKalbertodt
Copy link
Member

In the thread for RFC 2570, @Amanieu made a comment about better method names which everyone seemed to largely agree with. However, somehow it was forgotten to adjust the RFC. This PR fixes that. See the commit messages for more details.

There are some discussions about the exact API left. These can happen in the tracking issue. However, this change is probably a clear improvement over the current RFC. The RFC is also currently being implemented. CC @crlf0710

@LukasKalbertodt
Convert tabs to space in RFC 2570 Linked List Cursors
498c10a 
Tabs render as 8 spaces on GitHub which is different from the
standard Rust style (4 spaces).
@LukasKalbertodt
Rename some methods of Linked List Cursor
b486793 
These changes is where everyone seems to agree on.

- peek -> peek_next
- peek_before -> peek_prev
- insert -> insert_after
- insert_list -> splice_after
- insert_list_before -> splice_before
- split -> split_after
@LukasKalbertodt
Replace pop and pop_before with remove_current in Linked List C…
41a81ea 
…usors

The name "pop" was critized in the discussion thread but despite agreement
it should be changed, no one did. This commit implements basically the
suggestion by Amanieu (but `remove_current` instead of `remove`). There is
some discussion however, over if there should be two methods (differing in
what element is the current one after deletion). This should be discussed
in the tracking issue before stabilizing the feature.
@Amanieu Amanieu mentioned this pull request Jan 11, 2020
Merged
@clarfonthey
Copy link
Contributor

I thought that changes to names before stabilisation were usually done as part of the FCP process outside the RFC process.

I mean, it makes sense to make this change to the RFC text once everything is stabilised, but otherwise, I'm not really sure if an RFC is needed here.

@LukasKalbertodt
Copy link
Member Author

My idea was to change it since it was still discussed in the RFC thread and seems like it was just an accident to not adjust the RFC in time. But I'm totally fine with adjusting the RFC later on (e.g. when stabilizing).

@Amanieu
Copy link
Member

Amanieu commented Jan 12, 2020

I would like to make 2 additional changes:

  • Add an index method to cursors which returns an Option<usize>.
  • Change split_before and split_after to take &mut self. One property of these functions is that the current cursor remains valid and in the original list after a split, so it is not necessary to invalidate it.

@Amanieu
Copy link
Member

Amanieu commented Jan 12, 2020

Also I'm not a fan of the cursor/cursor_mut methods on LinkedList. I think the method name needs to explicitly say that the cursor points to the head of the list, and a method should be provided to get a cursor to the tail of the list.

Suggestions (pick one):

  • head/head_mut and tail/tail_mut, but might be ambiguous with the existing front/front_mut and back/back_mut which return references.
  • cursor_head/cursor_head_mut and cursor_tail/cursor_tail_mut, but might be a bit long to type.

@crlf0710 crlf0710 mentioned this pull request Jan 13, 2020
Open
4 tasks
@crlf0710
Copy link
Member

From the implementation side, currently i am choosing the second choice. Since there's no mention of head and tail yet in the LinkedList API, i'm changing the wording slightly to cursor_front/cursor_front_mut and cursor_back/cursor_back_mut accordingly.

@Amanieu
Copy link
Member

Amanieu commented Jan 14, 2020

@LukasKalbertodt If you update your PR to include the latest naming changes from the implementation then we can just merge it straight away.

@LukasKalbertodt
Copy link
Member Author

@Amanieu Should fit the initial implementation API now.

@Amanieu
Copy link
Member

Amanieu commented Jan 14, 2020

You forgot to change split_before and split_after to take &mut self.

@LukasKalbertodt
Rename a few more methods in Linked List Cursor RFC
772fb67 
These are just the names of the initially implementation.
@Mark-Simulacrum Mark-Simulacrum merged commit 3df38f6 into rust-lang:master Jan 14, 2020
@Mark-Simulacrum
Copy link
Member

(Merged per Discord request)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Amanieu Amanieu Amanieu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@LukasKalbertodt @clarfonthey @Amanieu @crlf0710 @Mark-Simulacrum