Replace manual iterator exhaust with for_each(drop) #48945

clarfonthey · 2018-03-12T00:52:18Z

This originally added a dedicated method, Iterator::exhaust, and has since been replaced with for_each(drop), which is more idiomatic.

~~This is just shorthand for for _ in &mut self {} or while let Some(_) = self.next() {}. This states the intent a lot more clearly than the identical code: run the iterator to completion.~~

~~At least personally, my eyes tend to gloss over for _ in &mut self {} without fully paying attention to what it does; having a Drop implementation akin to:~~

~~for _ in &mut self {}; unsafe { free(self.ptr); }~~

~~Is not as clear as:~~

~~self.exhaust(); unsafe { free(self.ptr); }~~

~~Additionally, I've seen debate over whether while let Some(_) = self.next() {} or for _ in &mut self {} is more clear, whereas self.exhaust() is clearer than both.~~

rust-highfive · 2018-03-12T00:52:29Z

r? @TimNN

(rust_highfive has picked a reviewer for you, use r? to override)

ollie27 · 2018-03-12T14:05:52Z

As I suggested in #45168 I think this should take self by value like all the other methods which consume an iterator.

Also the implementation should probably be written in terms of fold like #45168.

TimNN · 2018-03-12T20:14:47Z

Re-assigning to @rust-lang/libs:

r? @Kimundi

As I suggested in #45168 I think this should take self by value like all the other methods which consume an iterator.

I think that would kind of defeat the purpose of the method proposed here: Not dropping / consuming the iterator seems to be an explicit goal.

If consuming the iterator was the goal either mem::drop(iter) or iter.for_each(|_| ()) should work. In fact, .exhaust() would probably be equivalent to (&mut iter).for_each(|_| ()), I think.

clarfonthey · 2018-03-12T20:17:46Z

Note that |_| () could be worded as drop also.

ExpHP · 2018-03-13T02:33:35Z

And putting it all together with by_ref():

iter.by_ref().for_each(drop);
unsafe { free(self.ptr); }

I rather like the looks of what we can already write.

Centril · 2018-03-14T16:46:42Z

Is this operation common enough to warrant a dedicated method in Iterator?

sfackler · 2018-03-14T17:18:38Z

I think I agree that as_ref + for_each sufficiently covers this kind of thing.

bors · 2018-03-15T18:57:06Z

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

pietroalbini · 2018-03-20T08:20:53Z

Ping from triage @Kimundi! This PR (probably) needs a review.

@sfackler so, should this PR be closed?

clarfonthey · 2018-03-20T19:32:50Z

(If there is a motion to close I'll open a new PR to replace the instances that I changed to exhaust to by_ref().for_each(drop))

pietroalbini · 2018-03-26T14:11:26Z

Ping from triage @rust-lang/libs! Can we get a review/decision on this PR?

Kimundi · 2018-03-26T21:58:12Z

Sorry, seems to have slipped under my radar. I think that I agree with @ExpHP that .by_ref().for_each(drop) is probably the better solution, as it just uses existing functionality.

I personally don't have any strong feelings about wether to add the new method or not, but I know that something like exhaust() method has been proposed multiple times in the past already, and not been added even before we had .for_each(), so I'd tentatively counts that as a argument against it.

Centril · 2018-03-26T22:12:55Z

@Kimundi Can we make this a matter of documentation then and include text about .by_ref().for_each(drop) somewhere on Iterator?

scottmcm · 2018-03-26T22:21:25Z

somewhere on Iterator

It feels mure like a "common patterns" document more than the doc comment. Is there a good place for that in one of the books? RBE is on doc.RLO now; maybe there?

Centril · 2018-03-26T22:22:45Z

cc @steveklabnik ^

clarfonthey · 2018-04-04T23:13:38Z

I've replaced instances of the method with for_each(drop) as preferred by everyone here.

scottmcm · 2018-04-04T23:48:21Z

src/libstd/collections/hash/table.rs

@@ -1129,7 +1129,7 @@ impl<'a, K, V> ExactSizeIterator for Drain<'a, K, V> {

 impl<'a, K: 'a, V: 'a> Drop for Drain<'a, K, V> {
    fn drop(&mut self) {
-        for _ in self {}
+        self.for_each(drop);


Ooh, nice that this just works without needing .by_ref() 🙂

When I saw this comment my immediate thought was to hope that the CI actually passed, because I was pretty sure it would work but not 100%. :p

pietroalbini · 2018-04-09T09:55:58Z

Ping from triage @Kimundi! If I understood correctly this PR still needs your review.

pietroalbini · 2018-04-16T10:32:51Z

Ping from triage! Can @Kimundi (or someone else from @rust-lang/libs) review this?

Kimundi · 2018-04-16T10:53:02Z

@bors r+

bors · 2018-04-16T10:53:03Z

📌 Commit 5c58eec has been approved by Kimundi

bors · 2018-04-16T13:22:05Z

⌛ Testing commit 5c58eec with merge 1ef1563...

Replace manual iterator exhaust with for_each(drop) This originally added a dedicated method, `Iterator::exhaust`, and has since been replaced with `for_each(drop)`, which is more idiomatic. <del>This is just shorthand for `for _ in &mut self {}` or `while let Some(_) = self.next() {}`. This states the intent a lot more clearly than the identical code: run the iterator to completion. <del>At least personally, my eyes tend to gloss over `for _ in &mut self {}` without fully paying attention to what it does; having a `Drop` implementation akin to: <del>`for _ in &mut self {}; unsafe { free(self.ptr); }`</del> <del>Is not as clear as: <del>`self.exhaust(); unsafe { free(self.ptr); }` <del>Additionally, I've seen debate over whether `while let Some(_) = self.next() {}` or `for _ in &mut self {}` is more clear, whereas `self.exhaust()` is clearer than both.

bors · 2018-04-16T16:07:05Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Kimundi
Pushing 1ef1563 to master...

rust-highfive assigned TimNN Mar 12, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 12, 2018

clarfonthey force-pushed the iter_exhaust branch 5 times, most recently from 049a71c to f109596 Compare March 12, 2018 02:03

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

rust-highfive assigned Kimundi and unassigned TimNN Mar 12, 2018

Replace manual iter exhaust with for_each(drop).

5c58eec

clarfonthey force-pushed the iter_exhaust branch from f109596 to 5c58eec Compare April 4, 2018 23:10

clarfonthey changed the title ~~Add Iterator::exhaust.~~ Replace manual iterator exhaust with for_each(drop) Apr 4, 2018

scottmcm approved these changes Apr 4, 2018

View reviewed changes

sfackler mentioned this pull request Apr 16, 2018

Add Iterator::exhaust #49990

Closed

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 Apr 16, 2018

bors mentioned this pull request Apr 16, 2018

Stabilize x86/x86_64 SIMD #49664

Merged

bors merged commit 5c58eec into rust-lang:master Apr 16, 2018

varkor mentioned this pull request Apr 16, 2018

Improve eagerly evaluating an iterator for its side-effects #44546

Closed

clarfonthey deleted the iter_exhaust branch April 16, 2018 17:29

varkor mentioned this pull request Aug 20, 2018

The Great Generics Generalisation: HIR Followup #51880

Merged

pnkfelix mentioned this pull request Dec 3, 2018

NLL error on closure, but not on equivalent function #55526

Closed

scottmcm mentioned this pull request Sep 4, 2019

Adding method to consume an iterator efficiently #64117

Closed

scottmcm mentioned this pull request Apr 19, 2022

Enable draining iterator/stream elements into a () #94166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace manual iterator exhaust with for_each(drop) #48945

Replace manual iterator exhaust with for_each(drop) #48945

clarfonthey commented Mar 12, 2018 •

edited

Loading

rust-highfive commented Mar 12, 2018

ollie27 commented Mar 12, 2018

TimNN commented Mar 12, 2018

clarfonthey commented Mar 12, 2018

ExpHP commented Mar 13, 2018

Centril commented Mar 14, 2018

sfackler commented Mar 14, 2018

bors commented Mar 15, 2018

pietroalbini commented Mar 20, 2018

clarfonthey commented Mar 20, 2018

pietroalbini commented Mar 26, 2018

Kimundi commented Mar 26, 2018

Centril commented Mar 26, 2018

scottmcm commented Mar 26, 2018

Centril commented Mar 26, 2018

clarfonthey commented Apr 4, 2018

scottmcm Apr 4, 2018

clarfonthey Apr 5, 2018

pietroalbini commented Apr 9, 2018

pietroalbini commented Apr 16, 2018

Kimundi commented Apr 16, 2018

bors commented Apr 16, 2018

bors commented Apr 16, 2018

bors commented Apr 16, 2018

Replace manual iterator exhaust with for_each(drop) #48945

Replace manual iterator exhaust with for_each(drop) #48945

Conversation

clarfonthey commented Mar 12, 2018 • edited Loading

rust-highfive commented Mar 12, 2018

ollie27 commented Mar 12, 2018

TimNN commented Mar 12, 2018

clarfonthey commented Mar 12, 2018

ExpHP commented Mar 13, 2018

Centril commented Mar 14, 2018

sfackler commented Mar 14, 2018

bors commented Mar 15, 2018

pietroalbini commented Mar 20, 2018

clarfonthey commented Mar 20, 2018

pietroalbini commented Mar 26, 2018

Kimundi commented Mar 26, 2018

Centril commented Mar 26, 2018

scottmcm commented Mar 26, 2018

Centril commented Mar 26, 2018

clarfonthey commented Apr 4, 2018

scottmcm Apr 4, 2018

Choose a reason for hiding this comment

clarfonthey Apr 5, 2018

Choose a reason for hiding this comment

pietroalbini commented Apr 9, 2018

pietroalbini commented Apr 16, 2018

Kimundi commented Apr 16, 2018

bors commented Apr 16, 2018

bors commented Apr 16, 2018

bors commented Apr 16, 2018

clarfonthey commented Mar 12, 2018 •

edited

Loading