Skip to content

Add retain_back (aka truncate_front) method to Deque#663

Merged
zeenix merged 6 commits into
rust-embedded:mainfrom
nullstalgia:deque_retain_back
Jun 13, 2026
Merged

Add retain_back (aka truncate_front) method to Deque#663
zeenix merged 6 commits into
rust-embedded:mainfrom
nullstalgia:deque_retain_back

Conversation

@nullstalgia

Copy link
Copy Markdown
Contributor

Howdy again!

I originally intended to do this after championing for the stabilization of alloc's truncate_front method, but it appears someone got to it before me!

Feeling inspired, I jumped right to the heapless implementation like I had done earlier last year for Deque::truncate, using the new name decided for the method in the stabilization PR.

Miri seems happy, and running through the logic on pen and paper it seems sound, but please do let me know if I missed anything or if some more exhaustive tests would be needed!

@zeenix zeenix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! As usual, I only have nitpicks to offer. :)

Comment thread src/deque.rs Outdated
Comment thread src/deque.rs Outdated
Comment thread src/deque.rs Outdated
Comment thread src/deque.rs Outdated
Comment thread src/deque.rs Outdated
@nullstalgia

nullstalgia commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

@zeenix Since this PR is essentially a continuation of the implementation I did for truncate, am I to apply these same suggested changes to that method as well?

The Safety comments (and overall implementation) I had there and here are essentially copies from the ones from the stdlib's implementation of VecDeque::truncate and truncate_front VecDeque::retain_back. Otherwise there would be unexplained differences in the implementations despite how logically similar they are both between each other here and the stdlib's. Would that fall into the scope of a different PR?

@zeenix

zeenix commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

@zeenix Since this PR is essentially a continuation of the implementation I did for truncate, am I to apply these same suggested changes to that method as well?

Yes please. It's good to be always consistent but please in a separate PR than this.

The Safety comments (and overall implementation) I had there and here are essentially copies from the ones from the stdlib's implementation of VecDeque::truncate and truncate_front VecDeque::retain_back. Otherwise there would be unexplained differences in the implementations despite how logically similar they are both between each other here and the stdlib's.

The important bit is the logic and actual code. As long as they remain as similar as possible, I don't think deviating for improvement, is a problem.

Would that fall into the scope of a different PR?

This would be fixup of this umerged work, so I don't think so.

EDIT: I might have misunderstood you here. I mean, the fixups/changes to the unmerged changes in this PR, should go in this PR. Changes to existing code (already merged) should go into a separate one. I hope it's clear now. :)

@nullstalgia

Copy link
Copy Markdown
Contributor Author

Thanks for clarifying, I'll push the requested changes + submit an accompanying PR soon.

@zeenix zeenix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm otherwise.

Comment thread src/deque.rs
Comment thread src/deque.rs Outdated

@zeenix zeenix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok, time for merge then but before that, please rebase on master/main (instead of merging that) to keep the history easier to follow (and avoiding unrelated commits in the PR).

@nullstalgia

Copy link
Copy Markdown
Contributor Author

Alrighty, should be good to go! Thanks for helping see this PR through, hopefully they (t-libs) don't change the method name on us again! ;P

@zeenix zeenix added this pull request to the merge queue Jun 13, 2026
Merged via the queue into rust-embedded:main with commit 7899b83 Jun 13, 2026
21 checks passed
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.

2 participants