Skip to content

iterators: merge duplicate iterate methods for Reverse #58087

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Apr 12, 2025

The method bodies and state optional argument definition are exactly the same.

The only difference between the two methods, is that the more specific method, for Reverse{<:AbstractArray}, had a propagate_inbounds macro applied. I simply deleted the more specific method, together with the macro. Deleting the macro annotation is consistent with the fact that the iterate(::AbstractArray) method, in abstractarray.jl, has no such annotation.

@nsajko nsajko added arrays [a, r, r, a, y, s] iteration Involves iteration or the iteration protocol labels Apr 12, 2025
The method bodies and state optional argument definition are exactly
the same.

The only difference between the two methods, is that the more specific
method, for `Reverse{<:AbstractArray}`, had a `@propagate_inbounds`
annotation. I simply deleted the more specific method, together with
the annotation. Deleting the annotation is consistent with the fact
that the `iterate(::AbstractArray)` method, in abstractarray.jl, has no
such annotation.
@nsajko
Copy link
Contributor Author

nsajko commented Apr 16, 2025

bump

@jishnub
Copy link
Member

jishnub commented Apr 16, 2025

Does this impact performance in any way when applied to arrays in an inbounds context?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] iteration Involves iteration or the iteration protocol status: waiting for PR author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants