-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Make AdditiveIterator
and MultiplicativeIterator
extensible
#23293
Conversation
r? @huonw (rust_highfive has picked a reviewer for you, use r? to override) |
I suspect this change means that That said, there's deeper structure here: both of these consumers are operating on a monoid: an algebraic structure consisting an (associative) binary operator and an identity element for that operator. Other examples include appending strings/sequences/data structures in general. I idly wonder if approaching this concept from that more perspective may be better, instead of having two essentially identical traits. Thoughts, @aturon? |
@tbu- I'm curious what coherence problems you were running into? I presume trying to add a blanket impl of some kind? We used to have something along the lines @huonw is suggesting, where these worked for types satisfying |
@aturon The problem is that |
@huonw Any updates on this one? |
☔ The latest upstream changes (presumably #23936) made this pull request unmergeable. Please resolve the merge conflicts. |
@tbu-, what do you think of moving these methods onto the |
r? @aturon (transferring reviewership, don't have the bandwidth right now.) |
Previously it could not be implemented for types outside `libcore/iter.rs` due to coherence issues.
515c878
to
52bb3f7
Compare
@aturon I made |
📌 Commit 52bb3f7 has been approved by |
/// assert!(it.sum() == 15); | ||
/// ``` | ||
#[unstable(feature="core")] | ||
fn sum<T, S=T>(self) -> S where |
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.
Could this also omit the T
type parameter? I think something like this in theory shouldn't ICE:
fn sum<S=Self::Item>(self) -> S
where S: Add<Self::Item,Output=S> + Zero, Self: Sized
{
...
}
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.
@alexcrichton Indeed. Changing it right now.
Nice! |
52bb3f7
to
4b8a918
Compare
/// ``` | ||
#[unstable(feature="core")] | ||
fn sum<S=<Self as Iterator>::Item>(self) -> S where | ||
S: Add<Self::Item, Output=S> + Zero, |
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.
Does the output of Add
need to be the same as the Item
of the iterator? Could this be: fn sum(self) -> <Self::Item as Add>::Output where Self::Item: Add, Self::Item::Output: Zero
Not sure it's that worth it, but it'd allow summing when the output and input are different.
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.
The S=...
part is just setting a default for the type parameter. Iterator type and type that is being summed over can be different in the code.
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.
But the Output=S
part requires that the iterator's Item
and the Output
of Add
be the same
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.
Ohhh I understand it now :) Disregard!
⌛ Testing commit 4b8a918 with merge 82d1a5e... |
💔 Test failed - auto-mac-32-opt |
In addition to being nicer, this also allows you to use `sum` and `product` for iterators yielding custom types aside from the standard integers. Due to removing the `AdditiveIterator` and `MultiplicativeIterator` trait, this is a breaking change. [breaking-change]
@alexcrichton Sorry for the inconvenience, |
4b8a918
to
97f24a8
Compare
Previously it could not be implemented for types outside `libcore/iter.rs` due to coherence issues.
⌛ Testing commit 97f24a8 with merge dd6c4a8... |
Previously it could not be implemented for types outside
libcore/iter.rs
dueto coherence issues.