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

Implement VecDeque::retain_mut #91215

Merged
merged 1 commit into from
Dec 5, 2021
Merged

Implement VecDeque::retain_mut #91215

merged 1 commit into from
Dec 5, 2021

Conversation

GuillaumeGomez
Copy link
Member

Part of #90829.

In #90772, someone suggested that retain_mut should also be implemented on VecDeque. I think that it follows the same logic (coherency). So first: is it ok? Second: should I create a new feature for it or can we put it into the same one?

r? @joshtriplett

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 25, 2021
@GuillaumeGomez GuillaumeGomez added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 25, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472
Copy link
Member

the8472 commented Nov 25, 2021

If we add a new variant of retain it would be nice to add a range parameter, it helps in cases where one only wants to work on a sub-section of an vec, e.g. when a callee appended N items then it can be more efficient to only post-filter those items but not the whole vec.

@GuillaumeGomez
Copy link
Member Author

It seems like a completely different feature. You can't have more parameter in retain_mut than in retain. Maybe a retain_range or something along the line?

@the8472
Copy link
Member

the8472 commented Nov 25, 2021

Sure, the name could be different, but imo not having the range on retain was simply an oversight since one always could have done retain(.., |foo| {/* condition */}), so any new method should have it.

In this case &mut is a more powerful version because a &mut self is taken of the vec anyway, so if we're making a more powerful version we might as well add the range.

@GuillaumeGomez
Copy link
Member Author

Please open an issue so you can talk about it with libs team and not flood this PR too much.

@m-ou-se
Copy link
Member

m-ou-se commented Dec 4, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 4, 2021

📌 Commit 0466a12 has been approved by m-ou-se

@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 Dec 4, 2021
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 4, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2021
@bors
Auto merge of rust-lang#91539 - matthiaskrgr:rollup-rnl10yb, r=matthi…
5e93f6e 
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#89642 (environ on macos uses directly libc which has the correct signature.)
 - rust-lang#90022 (Explain why `Self` is invalid in generic parameters)
 - rust-lang#90023 (Postpone the evaluation of constant expressions that depend on inference variables)
 - rust-lang#91215 (Implement VecDeque::retain_mut)
 - rust-lang#91355 (std: Stabilize the `thread_local_const_init` feature)
 - rust-lang#91528 (LLVM support .insn directive)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4af985a into rust-lang:master Dec 5, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 5, 2021
@GuillaumeGomez GuillaumeGomez deleted the vec-deque-retain-mut branch December 5, 2021 11:23
@GuillaumeGomez GuillaumeGomez mentioned this pull request Feb 21, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@joshtriplett joshtriplett

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.
8 participants
@GuillaumeGomez @rust-log-analyzer @the8472 @m-ou-se @bors @joshtriplett @rust-highfive @rustbot