5

Add `vec::Drain{,Filter}::keep_rest` by WaffleLapkin · Pull Request #95376 · rus...

 1 year ago
source link: https://github.com/rust-lang/rust/pull/95376
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.

Conversation

Contributor

@WaffleLapkin WaffleLapkin commented on Mar 27 •

edited

This PR adds keep_rest methods to vec::Drain and vec::DrainFilter under drain_keep_rest feature gate:

// mod alloc::vec

impl<T, A: Allocator> Drain<'_, T, A> {
    pub fn keep_rest(self);
}

impl<T, F, A: Allocator> DrainFilter<'_, T, F, A>
where
    F: FnMut(&mut T) -> bool,
{
    pub fn keep_rest(self);
}

Both these methods cancel draining of elements that were not yet yielded from the iterators. While this needs more testing & documentation, I want at least start the discussion. This may be a potential way out of the "should DrainFilter exhaust itself on drop?" argument.

workingjubilee and TaKO8Ki reacted with hooray emojiworkingjubilee and Tamschi reacted with heart emoji All reactions

Collaborator

rust-highfive commented on Mar 27

r? @kennytm

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

rust-highfive

added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label

on Mar 27

Contributor

Gankra commented on Mar 27

I haven't looked at the code, but I think this is a better answer to 90% of what drain_filter is trying to do, and the appropriate release valve for people who don't like drain's default semantic of "you have already drained all of the elements, now we shall yield them one at a time". It is a very "Entry" solution to the problem in that it provides an external interface to drain_filter's messy internal one... oh god is that terminology anyone uses anymore. crumbles to dust
workingjubilee reacted with heart emoji All reactions

Contributor

Author

WaffleLapkin commented on Mar 27

Truth be told, when I struggled with the default behavior of drain_filter I've implemented a crappy entry/cursor API for Vec. I guess Entry-like APIs are just superior answer to all problems.

Contributor

bors commented on Apr 8

umbrella The latest upstream changes (presumably #95798) made this pull request unmergeable. Please resolve the merge conflicts.

Member

JohnCSimon commented on May 8

Ping from triage:
@WaffleLapkin can you please resolve the merge conflicts?

rustbot

added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label

on May 8

Contributor

the8472 commented on May 21

I think this should be the default for DrainFilter rather than an additional method.

Drain on the other hand already is stable so we can't change the default behavior and adding a method makes more sense there. But it's awkward ergonomically because those are iterators and you're presumably using them rather than retain() because you actually need the iterator part so you'd have to take it by_ref().

Perhaps adding a defaulted const generic Drain<T, const KEEP: bool = false> and keep_rest(self) -> Drain<T, true> would be more ergonomic.

the8472

added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

and removed T-libs Relevant to the library team, which will review and decide on the PR/issue.

labels

on May 21

Contributor

the8472 commented on May 21

r? rust-lang/libs-api

Contributor

bors commented on Jun 4

umbrella The latest upstream changes (presumably #97729) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon

added 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

on Jul 3

Member

@dtolnay dtolnay left a comment

I am on board with this API. Next steps are:

  • I think this is the kind of method that would benefit from a usage example in the rustdoc.
  • Needs a tracking issue.

Some considerations to note in the tracking issue for discussion before we could stabilize:

  • Just change the not-yet-stable Drop for DrainFilter to keep rest?
    • Advantage: usually what you want (??)
      • I don't have a good picture what the use cases involve drain_filter and not exhausting the iterator obtained from it.
    • Disadvantage: inconsistent with stable Drop for Drain
    • If you want to remove rest (matching the current unstable behavior of drain_filter) then you'd need to write .foreach(drop) to explicitly drop all the rest of the range that matches the filter.
  • &mut self instead of self?
    • If you're holding a Drain inside a struct and are operating on it from a &mut self method of the struct, keep_rest(self) is impossible to use. :(
      • You'd want something like mem::replace(&mut self.drain_filter, Vec::new().drain(..)).keep_rest() but the borrow checker won't like that.
      • Failing that, you'd need to change your Drain field to Option<Drain> and use self.drain_filter.take().unwrap().keep_rest() along with unwrap() everywhere else that the drain is used. Not good.
    • We'd need to define behavior of calling .next() after .keep_rest(). Presumably one of:
      • .next() returns None
      • this is considered incorrect usage and so .next() panicks
      • .keep_rest() sets a flag inside the Drain which Drop will use to decide its behavior, and you're free to continue accessing iterator elements in between.
All reactions

Contributor

Author

WaffleLapkin commented 11 days ago

I've updated docs and the tracking issue white_check_mark

Member

@dtolnay dtolnay left a comment

Thanks!

All reactions

Member

dtolnay commented 10 days ago

@bors r+

Contributor

bors commented 10 days ago

pushpin Commit 7a433e4 has been approved by dtolnay

It is now in the queue for this repository.

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

10 days ago

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dtolnay

Assignees

dtolnay

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.65.0

Development

Successfully merging this pull request may close these issues.

None yet

9 participants

report

Recommend

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK