TrustedRandomAccess specialisation for Iterator::cloned when Item: Copy. #44790
Conversation
|
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
clarfonthey
force-pushed
the
zip_bytes
branch
3 times, most recently
from
9870913 to
71c62cb
Compare
This verifies that TrustedRandomAccess has no side effects when the iterator item implements Copy. This also implements TrustedLen and TrustedRandomAccess for str::Bytes.
clarfonthey
force-pushed
the
zip_bytes
branch
from
71c62cb to
1c589b7
Compare
| @@ -499,6 +499,18 @@ unsafe impl<'a, I, T: 'a> TrustedRandomAccess for Cloned<I> | |||
| fn may_have_side_effect() -> bool { true } | |||
| } | |||
|
|
|||
| #[doc(hidden)] | |||
| unsafe impl<'a, I, T: 'a> TrustedRandomAccess for Cloned<I> | |||
There was a problem hiding this comment.
Why is this specialization required? It seems totally equivalent to the impl above.
There was a problem hiding this comment.
It returns false for may_have_side_effect.
There was a problem hiding this comment.
Maybe may_have_side_effect could be removed and it just assumes everything has side effects. Since some .map().zip() invocations optimize ok it could be suspected that the compiler can remove dead code itself. Unfortunately a closer examination of benchmarks is needed.
There was a problem hiding this comment.
This does assume that Clone is simply *self, but it could do weird side-effectful things. I think it's probably reasonable to state that we're allowed to assume that Clone behaves sanely for Copy types, but I don't think that's explicitly documented currently.
There was a problem hiding this comment.
It appears to be in fact documented: https://doc.rust-lang.org/std/clone/trait.Clone.html#how-can-i-implement-clone
|
may_have_side_effect is a crutch for .zip(): by default it calls .next() on the LHS once more than on the RHS, to discover that the LHS is at its end. The specialization aims to preserve this by default, so that nobody notices anything. (Edited) This whole crutch is actually needed, for when the input iterators are different lengths. |
|
@bors r+ |
|
📌 Commit 1c589b7 has been approved by |
carols10cents
added
the
S-waiting-on-bors
TrustedRandomAccess specialisation for Iterator::cloned when Item: Copy. This should fix #44424. It also provides a potential fix for more iterators using `Iterator::cloned`.
|
☀️ Test successful - status-appveyor, status-travis |
|
@bors try- retry rollup I don't know what bors is doing after the last comment... |
This should fix #44424. It also provides a potential fix for more iterators using
Iterator::cloned.