-
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
Use min_specialization in libcore #73565
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
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.
Only made a very basic initial pass, got distracted before I could go through the whole PR.
src/libcore/iter/adapters/zip.rs
Outdated
#[doc(hidden)] | ||
#[unstable(feature = "trusted_random_access", issue = "none")] | ||
#[rustc_specialization_trait] | ||
pub unsafe trait TrustedRandomAccess: 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.
Why does this need to become pub
?
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's now used in the interface of Iterator
src/libcore/iter/adapters/zip.rs
Outdated
/// | ||
/// `size` may not be overridden. | ||
/// | ||
/// `<Self as Iterator>::get_unchecked` must be overridden. It must be safe to |
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.
Given that the default implementation simply panic!()
s, this is not an unsafety invariant, is it?
☔ The latest upstream changes (presumably #73643) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot modify labels to: +S-waiting-on-author -S-waiting-on-review |
Let's r? @nagisa as they left some review comments. |
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.
@bors r+
src/libcore/tests/iter.rs
Outdated
assert_eq!(iter.next_back(), Some((40, 5000))); | ||
assert_eq!(iter.next_back(), Some((30, 4000))); | ||
assert_eq!(a, vec![6, 5, 4, 3]); | ||
assert_eq!(b, vec![800, 700, 600, 500, 400]); |
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.
Do we actually want to enforce that the 2nd iterator consumes 1 more value than from the first iterator? I think this could go either way, but I would at least leave a comment to that effect so that a change in this behaviour wouldn’t immediately appear invalid.
Oh well, this will need a rebase, but r=me once that's done. Sorry for taking so long to get around to this. |
66b60da
to
1a4d826
Compare
1a4d826
to
dbad8c9
Compare
@bors r=nagisa |
📌 Commit dbad8c9 has been approved by |
⌛ Testing commit dbad8c9 with merge 1dc99bb058f144c8f4f64b005e546c10f301901b... |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - checks-actions |
@bors retry |
⌛ Testing commit dbad8c9 with merge 2f15b6b842776d00d2eb9369deeb7dcd1b3a3baa... |
💔 Test failed - checks-azure |
@bors retry |
☀️ Test successful - checks-actions, checks-azure |
@matthewjasper I'm currently thinking how to extend |
|
Getting
TrustedRandomAccess
to work is the main interesting thing here.get_unchecked
is now an unstable, hidden method onIterator
TrustedRandomAccess
is made clearer in documentationDebug
would create aliasing references when using the specialized zip implnext_back
andnth
.closes #68536