Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement a specialized version std::iter::Enumerate for TrustedLen #77822
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
Implement a specialized version std::iter::Enumerate for TrustedLen #77822
Changes from all commits
708e9bf
44c5554
49b8f87
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@matthewjasper Could you comment on the possibilities here? Specializing on
ExactSizeIterator
would be useful here, but is not accepted. I see#[rustc_specialization_trait]
and#[rustc_unsafe_specialization_marker]
being used in some places to work around this, but I don't know in which cases those can be used.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.
Considering that
TrustedLen
is unsafe + unstable +rustc_unsafe_specialization_marker
it is more or less implied that any type that implements it is some iterator owned by the standard library. Which means there are no lifetime shenanigans that would makeExactSizeIterator
unsafe for min_specialization.Using that justification a
rustc_unsafe_specialization_marker
helper trait inheriting from both should be fine. Probably.On the other hand
TrustedRandomAccess
is extending its tendrils into ever more places, so soon it should be almost equivalent in power toTrustedLen + ExactSizeIterator
.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.
TrustedRandomAccess
seems like it would always be more specific thanTrustedLen+ExactSizeIterator
. Implementing the former on things likeBTreeMap
would probably be not a good idea, whileTrustedLen+ExactSizeIterator
would be easy.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.
Then yeah, I think that should be possible. But it might be good to get a second opinion from someone who has better understanding of the min_specialization rules.
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.
Considering the progress that happened in the LLVM bug report and all the issues found recently involving
TrustedRandomAccess
, maybe let's just wait this out until LLVM handles it correctly?What do you think @m-ou-se ?
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 use
Add::add
instead of+
? If this is necessary, a comment explaining why would be useful.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.
That's how it was originally and is everywhere else in this file. I don't know why and was wondering the same, but kept it because there presumably was a reason
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
Add::add
delays decision about overflow check mode to the crate that does the code generation, even if overflow checks are disabled in the current crate, which they usually are for the standard library.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.
According to #81721, we should be using
#[rustc_inherit_overflow_checks]
with a regular+
operator here instead of theAdd:add
trick.