-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Add fold_mut alternative to Iterator trait #76746
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1990,6 +1990,44 @@ pub trait Iterator { | |
accum | ||
} | ||
|
||
/// An iterator method that applies a function, producing a single, final value. | ||
/// | ||
/// `fold_mut()` is very similar to [`fold()`] except that the closure | ||
/// takes a `&mut` to the 'accumulator' and does not need to return a new value. | ||
/// | ||
/// [`fold()`]: Iterator::fold | ||
/// | ||
/// # Examples | ||
/// | ||
/// Basic usage: | ||
/// | ||
/// ``` | ||
/// #![feature(iterator_fold_mut)] | ||
/// use std::collections::HashMap; | ||
/// | ||
/// let word = "abracadabra"; | ||
/// | ||
/// // the count of each letter in a HashMap | ||
/// let counts = word.chars().fold_mut(HashMap::new(), |map, c| { | ||
/// *map.entry(c).or_insert(0) += 1; | ||
/// }); | ||
/// | ||
/// assert_eq!(counts[&'a'], 5); | ||
/// ``` | ||
#[inline] | ||
#[unstable(feature = "iterator_fold_mut", issue = "76725")] | ||
fn fold_mut<B, F>(mut self, init: B, mut f: F) -> B | ||
where | ||
Self: Sized, | ||
F: FnMut(&mut B, Self::Item), | ||
{ | ||
let mut accum = init; | ||
while let Some(x) = self.next() { | ||
f(&mut accum, x); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the default should be implemented in terms of self.fold(init, |mut acc, x| { f(&mut acc, x); acc }) Even better if that isolates the closure generics like #62429, so it only depends on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Try with inputs like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I like this idea! I tried it here and the results seem to indicate that this brought back the performance "issue" (assuming the benchmark is built right, Criterion is doing it's thing correctly, and I'm interpreting the results correctly): Should I push forward with that? I was originally making
Good call, I'll whip up a few of these when I get the chance! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eureka! I added some benchmarks (with terrible names and with terrible duplication that should be replaced with a macro) in 2921101 and it appears that Unfortunately they're a little bit out of order - I will definitely pick better names if we decide to go through with this PR but basically for For a non-chain-non-flat-map fold on a hashmap, they're about the same (except For a non-chain-non-flat-map fold on a number, For a non-chain-non-flat-map fold on a I think maybe I'll port these benchmarks back to my other repo so I can use criterion for (maybe) more consistent data? Or should we just pull the plug on this? Or redirect the effort towards the ergonomics and not worry about performance? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's expected because of the let mut accum = init;
- while let Some(x) = self.next() {
- f(&mut accum, x);
- }
+ self.for_each(|x| f(&mut accum, x)); (insert comment about #62429 here and how that shouldn't be the implementation in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm that went a little over my head, let me see if I understand:
Interested to know if any of those are in the right ballpark! And for expedience (maybe), assuming those points are in the correct ballpark, may I ask:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The concerns of #62429 are mostly separate from the concerns of The point of #62429 is that closures inherit all of the generic type parameters of the surrounding scope. In your example that would be The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, thank you for that explanation!
Awesome, this makes a ton of sense
Woah, did not see that coming! So I originally read that thinking, "Great, I'll rewrite I'm running out of benchmark steam (is there a better way to do this than running
And ran 8 benchmarks. 4 were operating on
so I think based on all these shifty benchmarks moving around so much ... maybe these're all within statistical uncertainy (combined with whatever my computer is doing at any given time). This was super fun to play around with but I think I'm going to bow out of the "maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's a zero sized type (ZST), so there's literally nothing to do in codegen. I'm really surprised that it did poorly in your benchmark, but my guess is that there was some unlucky inlining (or lack thereof), especially if parts of that benchmark got split across codegen-units. |
||
accum | ||
} | ||
|
||
/// The same as [`fold()`], but uses the first element in the | ||
/// iterator as the initial value, folding every subsequent element into it. | ||
/// If the iterator is empty, return [`None`]; otherwise, return the result | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
issue
needs to be a real tracking issue, not the feature request.See other instances of issues labeled
C-tracking-issue
andT-libs
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I created #76751 but I don't obviously see how to add T-libs to it 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It got tagged but in the future you can use @rustbot modify labels: +(label), -(label)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d7cc6be