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

Implement Vec::pop_if #123107

Merged
merged 1 commit into from Mar 27, 2024
Merged

Implement Vec::pop_if #123107

merged 1 commit into from Mar 27, 2024

Conversation

avandesa
Copy link
Contributor

This PR adds Vec::pop_if to the public API, behind the vec_pop_if feature.

impl<T> Vec<T> {
    pub fn pop_if<F>(&mut self, f: F) -> Option<T>
        where F: FnOnce(&mut T) -> bool;
}

Tracking issue: #122741

Open questions

  • Should the first unit test be split up?
  • I don't see any guidance on ordering of methods in impl blocks, should I move the method elsewhere?

@rustbot
Copy link
Collaborator

rustbot commented Mar 26, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joboet (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 26, 2024
Copy link
Contributor

@joboet joboet left a comment

Choose a reason for hiding this comment

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

If the predicate modifies the value but returns false, that should be reflected in the vector. Hence this should call f with the value still in the vector, and only then remove it. Also, I'd prefer if this didn't use unsafe, I'd wager that the optimizer is able to deal with a combination of slice::last and Vec::pop just fine.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2024
@avandesa avandesa marked this pull request as ready for review March 26, 2024 21:09
@avandesa
Copy link
Contributor Author

Fair enough, I'll trust your judgement on the optimizer. Switch to last_mut and pop and added a test for an impure predicate.

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 26, 2024
Copy link
Contributor

@joboet joboet left a comment

Choose a reason for hiding this comment

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

Looks great, thank you very much! Could you squash the commits, please?

Then r=me
(this shorthand tells other people with merge rights that they can merge this on my behalf if don't get to it right away)

@avandesa
Copy link
Contributor Author

@bors r=joboet

@bors
Copy link
Contributor

bors commented Mar 26, 2024

@avandesa: 🔑 Insufficient privileges: Not in reviewers

@avandesa
Copy link
Contributor Author

Ope, I misunderstood that I think

@clubby789
Copy link
Contributor

@bors r=joboet

@bors
Copy link
Contributor

bors commented Mar 27, 2024

📌 Commit 07d3806 has been approved by joboet

It is now in the queue for this repository.

@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 Mar 27, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 27, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#122439 (match lowering: build the `Place` instead of keeping a `PlaceBuilder` around)
 - rust-lang#122880 (Unix: Support more platforms with `preadv` and `pwritev`)
 - rust-lang#123038 (std library thread.rs: fix NetBSD code for ILP32 CPUs.)
 - rust-lang#123084 (`UnixStream`: override `read_buf`)
 - rust-lang#123102 (RustWrapper: update call for llvm/llvm-project@44d037cc258dcf179d2c48…)
 - rust-lang#123107 (Implement `Vec::pop_if`)
 - rust-lang#123118 (Update `RwLock` deadlock example to not use shadowing)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 19a40ec into rust-lang:master Mar 27, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 27, 2024
Rollup merge of rust-lang#123107 - avandesa:vec_pop_if, r=joboet

Implement `Vec::pop_if`

This PR adds `Vec::pop_if` to the public API, behind the `vec_pop_if` feature.

```rust
impl<T> Vec<T> {
    pub fn pop_if<F>(&mut self, f: F) -> Option<T>
        where F: FnOnce(&mut T) -> bool;
}
```

Tracking issue: rust-lang#122741

## Open questions

- [ ] Should the first unit test be split up?
- [ ] I don't see any guidance on ordering of methods in impl blocks, should I move the method elsewhere?
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants