Skip to content
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

restrict last(v::AbstractArray, n::Integer) to AbstractVector #36642

Merged
merged 1 commit into from
Jul 15, 2020

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Jul 13, 2020

As explained in my comment in #34868, we should be consistent here.

… in general, this optimization only seems to make sense for arrays with linear indexing, but it's easiest to just use AbstractVector here and rely on the Iterators.reverse fallback for other cases.

cc @giordano

As explained in my comment https://github.com/JuliaLang/julia/pull/34868/files#r453380729

… in general, this optimization only seems to make sense for arrays with linear indexing, but it's easiest to just use AbstractVector here and rely on the `Iterators.reverse` fallback for other cases.
@giordano
Copy link
Contributor

I remember there were some cases where the specialised first method with a non-vector argument would have returned something unexpected, but I can't remember what exactly was the problem. Instead, I didn't find any issue with last, but I agree it's probably be safer to be conservative and restrict also this last method to vectors only

@JeffBezanson JeffBezanson merged commit aa40074 into master Jul 15, 2020
@JeffBezanson JeffBezanson deleted the stevengj-patch-5 branch July 15, 2020 19:36
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
…ang#36642)

As explained in my comment https://github.com/JuliaLang/julia/pull/34868/files#r453380729

… in general, this optimization only seems to make sense for arrays with linear indexing, but it's easiest to just use AbstractVector here and rely on the `Iterators.reverse` fallback for other cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants