Implement Iterator::fold for .chain(), .cloned(), .map() and the VecDeque iterators. #37315
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| @@ -1829,6 +1811,42 @@ fn wrap_index(index: usize, size: usize) -> usize { | |||
| index & (size - 1) | |||
| } | |||
|
|
|||
| /// Returns the two slices that cover the VecDeque's valid range | |||
| trait RingSlices : Sized { | |||
| fn slice(self, from: usize, to: usize) -> Self; | |||
There was a problem hiding this comment.
Could this be replaced with a bound on Index?
There was a problem hiding this comment.
No, we're using either Index or IndexMut
|
Github won't let me comment on lines you didn't change. You could also modify the fn select_fold1<I,B, FProj, FCmp>(it: I,
mut f_proj: FProj,
mut f_cmp: FCmp) -> Option<(B, I::Item)>
where I: Iterator,
FProj: FnMut(&I::Item) -> B,
FCmp: FnMut(&B, &I::Item, &B, &I::Item) -> bool
{
let mut projected = it.map(|x| (f_proj(&x), x));
projected.next().map(|(sel_p, sel)| {
projected.fold((sel_p, sel), |(sel_p, sel), (x_p, x)| {
if f_cmp(&sel_p, &sel, &x_p, &x) {
(x_p, x)
} else {
(sel_p, sel)
}
})
})
}
This should help |
|
|
|
As said on irc, I'd prefer to have add-on improvements in a separate PR. |
|
Makes sense, I'll create a spearate PR off my branch this gets merged. Very excited about this idea of using |
| fn slice(self, from: usize, to: usize) -> Self; | ||
| fn split_at(self, i: usize) -> (Self, Self); | ||
|
|
||
| fn ring_slices(buf: Self, head: usize, tail: usize) -> (Self, Self) { |
There was a problem hiding this comment.
Maybe some #[inline]-s here and on the slice/split_at methods below would help ensure the generated code stays the same as before? (don't have any data that it doesn't and since code is generic, it'll always be available to inline)
There was a problem hiding this comment.
Since it is generic it should not matter I think. It is sort of a higher level operation anyway.
bluss
changed the title
|
Some further validation for this idea: I specialised the |
|
Cool. The improvement of The reasoning for this PR is
|
|
So while |
|
@bors: r+ Nice wins! Neat idea propagating through adapters like |
|
📌 Commit f095f0d has been approved by |
|
⌛ Testing commit f095f0d with merge 8a2de11... |
|
💔 Test failed - auto-linux-64-debug-opt |
|
Thank you. I'll have to investigate this llvm assertion.. |
|
I ruled out the VecDeque and cloned(), .map() changes, so it must be .chain() or .flat_map() (causing the assertion). |
Implement .fold() specifically for .map() and .cloned() so that any inner fold improvements are available through map and cloned.
Chain can do something interesting here where it passes on the fold into its inner iterators. The lets the underlying iterator's custom fold() be used, and skips the regular chain logic in next.
bluss
changed the title
|
I haven't solved it, but I have removed the FlatMap::fold method that caused the debuginfo-related llvm assertion when building librustc. It turned out to be the This way the other improvements can go in first. The remaining parts should not cause any llvm assertions. |
|
@bors: r+ |
|
📌 Commit a16626f has been approved by |
Implement Iterator::fold for .chain(), .cloned(), .map() and the VecDeque iterators. Chain can do something interesting here where it passes on the fold into its inner iterators. The lets the underlying iterator's custom fold() be used, and skips the regular chain logic in next. Also implement .fold() specifically for .map() and .cloned() so that any inner fold improvements are available through map and cloned. The same way, a VecDeque iterator fold can be turned into two slice folds. These changes lend the power of the slice iterator's loop codegen to VecDeque, and to chains and flat maps of slice iterators, and so on. It's an improvement for .sum() and .product(), and other uses of fold.
Implement Iterator::fold for .chain(), .cloned(), .map() and the VecDeque iterators. Chain can do something interesting here where it passes on the fold into its inner iterators. The lets the underlying iterator's custom fold() be used, and skips the regular chain logic in next. Also implement .fold() specifically for .map() and .cloned() so that any inner fold improvements are available through map and cloned. The same way, a VecDeque iterator fold can be turned into two slice folds. These changes lend the power of the slice iterator's loop codegen to VecDeque, and to chains of slice iterators, and so on. It's an improvement for .sum() and .product(), and other uses of fold.
bluss
added
the
relnotes
Chain can do something interesting here where it passes on the fold
into its inner iterators.
The lets the underlying iterator's custom fold() be used, and skips the
regular chain logic in next.
Also implement .fold() specifically for .map() and .cloned() so that any
inner fold improvements are available through map and cloned.
The same way, a VecDeque iterator fold can be turned into two slice folds.
These changes lend the power of the slice iterator's loop codegen to
VecDeque, and to chains of slice iterators, and so on.
It's an improvement for .sum() and .product(), and other uses of fold.