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

Add Vec::retain_mut #83218

Closed
wants to merge 1 commit into from
Closed

Add Vec::retain_mut #83218

wants to merge 1 commit into from

Conversation

jonas-schievink
Copy link
Contributor

Take 2 of #34265, since I needed this today.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2021
@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

I think the interaction with drain_filter and the (to me) very similar API surface is interesting here; I'm not sure whether one is definitively easier than the other to stabilize, and while both are unstable it feels like drain_filter already gives basically the same API? (Potentially at higher cost but hopefully not...)

r? @m-ou-se for T-libs member approval of unstable API

@m-ou-se
Copy link
Member

m-ou-se commented Mar 18, 2021

@jonas-schievink I'd like to hear your opinion on the (unstable) drain_filter as an alternative, since this already works:

#![feature(drain_filter)]

let mut vec = vec![1, 2, 3, 4];
vec.drain_filter(|x| if *x > 3 {
    true
} else {
    *x += 1;
    false
});
assert_eq!(vec, [2, 3, 4]);

That function is still unstable, so if there is a specific reason you don't consider it suitable for this use case, maybe we can come up with a way to change it such that it would suffice.

@jonas-schievink
Copy link
Contributor Author

Yeah, drain_filter would work as well here, that's true. I don't think there's any reason retain_mut would be required there.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 21, 2021

@jonas-schievink So, should we close this? Or is there a reason you'd still prefer retain_mut over drain_filter?

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2021
@jonas-schievink
Copy link
Contributor Author

Yeah, I guess let's close

@GuillaumeGomez
Copy link
Member

I would love to have retain_mut because in my case, I'm not interested by the removed items. Would it be fine to re-open this PR or should I open another one (with this code I think, doesn't seem to need to have something else). Or do I need to open an issue or something along the line before?

@Mark-Simulacrum
Copy link
Member

Even if you don't need the removed items, drain_filter still provides a near-equivalent API, so it's not clear that it's any worse to me. Having two APIs doing the same thing with slightly different interfaces seems worse, though.

That said, I think just a new PR here is fine, if you want to discuss that.

@GuillaumeGomez
Copy link
Member

Well, it would be coherent with the rest. For example we have chunks and chunks_mut or get and get_mut or iter and iter_mut, etc. When looking for mutable retain, I would expect retain_mut to exist. It took me a while to find out about drain_filter. So even if it provides an API close to drain_filter, just for the discoverability, I think it's worth it.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 16, 2021
…shtriplett

Add Vec::retain_mut

This is to continue the discussion started in rust-lang#83218.

Original comment was:

> Take 2 of rust-lang#34265, since I needed this today.

The reason I think why we should add `retain_mut` is for coherency and for discoverability. For example we have `chunks` and `chunks_mut` or `get` and `get_mut` or `iter` and `iter_mut`, etc. When looking for mutable `retain`, I would expect `retain_mut` to exist. It took me a while to find out about `drain_filter`. So even if it provides an API close to `drain_filter`, just for the discoverability, I think it's worth it.

cc `@m-ou-se` `@jonas-schievink` `@Mark-Simulacrum`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 16, 2021
…shtriplett

Add Vec::retain_mut

This is to continue the discussion started in rust-lang#83218.

Original comment was:

> Take 2 of rust-lang#34265, since I needed this today.

The reason I think why we should add `retain_mut` is for coherency and for discoverability. For example we have `chunks` and `chunks_mut` or `get` and `get_mut` or `iter` and `iter_mut`, etc. When looking for mutable `retain`, I would expect `retain_mut` to exist. It took me a while to find out about `drain_filter`. So even if it provides an API close to `drain_filter`, just for the discoverability, I think it's worth it.

cc ``@m-ou-se`` ``@jonas-schievink`` ``@Mark-Simulacrum``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 16, 2021
…shtriplett

Add Vec::retain_mut

This is to continue the discussion started in rust-lang#83218.

Original comment was:

> Take 2 of rust-lang#34265, since I needed this today.

The reason I think why we should add `retain_mut` is for coherency and for discoverability. For example we have `chunks` and `chunks_mut` or `get` and `get_mut` or `iter` and `iter_mut`, etc. When looking for mutable `retain`, I would expect `retain_mut` to exist. It took me a while to find out about `drain_filter`. So even if it provides an API close to `drain_filter`, just for the discoverability, I think it's worth it.

cc ```@m-ou-se``` ```@jonas-schievink``` ```@Mark-Simulacrum```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 16, 2021
…shtriplett

Add Vec::retain_mut

This is to continue the discussion started in rust-lang#83218.

Original comment was:

> Take 2 of rust-lang#34265, since I needed this today.

The reason I think why we should add `retain_mut` is for coherency and for discoverability. For example we have `chunks` and `chunks_mut` or `get` and `get_mut` or `iter` and `iter_mut`, etc. When looking for mutable `retain`, I would expect `retain_mut` to exist. It took me a while to find out about `drain_filter`. So even if it provides an API close to `drain_filter`, just for the discoverability, I think it's worth it.

cc ````@m-ou-se```` ````@jonas-schievink```` ````@Mark-Simulacrum````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 17, 2021
…shtriplett

Add Vec::retain_mut

This is to continue the discussion started in rust-lang#83218.

Original comment was:

> Take 2 of rust-lang#34265, since I needed this today.

The reason I think why we should add `retain_mut` is for coherency and for discoverability. For example we have `chunks` and `chunks_mut` or `get` and `get_mut` or `iter` and `iter_mut`, etc. When looking for mutable `retain`, I would expect `retain_mut` to exist. It took me a while to find out about `drain_filter`. So even if it provides an API close to `drain_filter`, just for the discoverability, I think it's worth it.

cc `````@m-ou-se````` `````@jonas-schievink````` `````@Mark-Simulacrum`````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 17, 2021
…shtriplett

Add Vec::retain_mut

This is to continue the discussion started in rust-lang#83218.

Original comment was:

> Take 2 of rust-lang#34265, since I needed this today.

The reason I think why we should add `retain_mut` is for coherency and for discoverability. For example we have `chunks` and `chunks_mut` or `get` and `get_mut` or `iter` and `iter_mut`, etc. When looking for mutable `retain`, I would expect `retain_mut` to exist. It took me a while to find out about `drain_filter`. So even if it provides an API close to `drain_filter`, just for the discoverability, I think it's worth it.

cc ``````@m-ou-se`````` ``````@jonas-schievink`````` ``````@Mark-Simulacrum``````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants