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

libcollections: add seekers to DList #16396

Closed
wants to merge 2 commits into from
Closed

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Aug 10, 2014

Iterator semantics... really don't make sense with a linked list. If you want to seek into the list, and then start manipulating it freely, seeking back and forth as needed, which is the entire usecase for a linked list, then an iterator that "consumes" its range just doesn't make sense.

So here's a "seeker" which lets you actually do the things that Linked Lists do well. Some things refactored along the way to reduce some duplication. There's probably some more places that could be refactored after adding this.

@Gankra Gankra changed the title add seekers to DList libcollections: add seekers to DList Aug 10, 2014
@Gankra
Copy link
Contributor Author

Gankra commented Aug 10, 2014

When the seek head is "off" the list, it's a bit ambiguous what insert/pop should do. For simplicity I will simply refer to the leftOfHead case, where the seeker points to "before" the first element. Here insert_after and pop_after seem obvious, they manipulate the head of the list. However, insert_before and pop_before are less clear. For pop_before it seemed reasonable to just yield None. For insert_before at the time of writing it made sense to just handle it the same as insert_after: append to the front of the list. However upon reflection this strikes me as potentially confusing, as it breaks some reasonable expectations of how you would expect insert_before to interact. Elements appear to the right of the seek head, even though they were inserted "before" it.

@aturon
Copy link
Member

aturon commented Aug 13, 2014

This is definitely useful functionality, sometimes known as a "Cursor", which likely will apply to other collections as well.

I wonder whether, instead of introducing a separate notion of "Seeker", the mutating methods could be added directly to the mutable iterator for DList? Note that the DList iterators are already double-ended, which means you can peel off items from either end of the range (which means that immutable seekers are, I think, wholly redundant). Adding methods to MutItems would make this change more appealing by reducing duplication.

@steveklabnik
Copy link
Member

I would 👍 on "Cursor" rather than "Seeker"

@Gankra
Copy link
Contributor Author

Gankra commented Aug 13, 2014

I wrote this in a very conservative manner under the assumption we didn't want to undermine the current notion of DoubleEndedIterators "consuming" their range irreversibly. I'm not opposed to it, though; less different iteration objects is definitely an ergonomic win.

However, it would definitely complicate Cursors. I designed cursors so that it's impossible to invalidate the value pointed to by the cursor, which makes them very easy to reason about. But with a double-ended cursor, you need to handle more edge cases about what happens when both heads are adjacent. Especially if you want to still have them work like DEI's. I suppose from the perspective of each cursor, the other represents the "end" of the list? You would get some awkward behaviour with immutable cursors where moving one cursor can change the logical "value" pointed to by the other (i.e. change it between None and Some). You also have to decide what happens when you delete the node pointed to by a cursor.

Regardless, most of the work is thankfully done regardless of how we proceed because of my refactoring.

@arthurprs
Copy link
Contributor

Something related that we also need badly is a way to return a Cursor where the item was inserted. It's very useful when building a LRU data structure for example.

It would be nice to integrate these features into the existing iterators like @aturon suggested.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 24, 2014

Closing as this quite out of date. Will rebase/refactor offline, and come back with a version that simply extends iterators.

@Gankra Gankra closed this Aug 24, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2024
fix panic with reference in macro

it panic at `builder.make_mut(segment)`, where segment is from macro expand. And the usage reference in orginal macro call isn't a `PathSegment` so we can't update it in `apply_references`, I can't find a way to deal with it properly so here just filter out the reference in macro. LMK if there are better way to fix this

try to close rust-lang/rust-analyzer#16328
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.

None yet

4 participants