More fixes for non-1 arrays #21251
More fixes for non-1 arrays #21251
Conversation
|
@nanosoldier |
| @@ -124,7 +124,7 @@ julia> length(A) | |||
| length(t::AbstractArray) = (@_inline_meta; prod(size(t))) | |||
| _length(A::AbstractArray) = (@_inline_meta; prod(map(unsafe_length, indices(A)))) # circumvent missing size | |||
| _length(A) = (@_inline_meta; length(A)) | |||
| endof(a::AbstractArray) = (@_inline_meta; length(a)) | |||
| endof(a::AbstractArray) = (@_inline_meta; last(linearindices(a))) | |||
There was a problem hiding this comment.
This will allow things like A[1:end] for offset arrays, but that's not tested. Is that intended? Of course A[end, 1] will still error with a missing size.
There was a problem hiding this comment.
Ideally we'd want A[begin:end] but of course that would need parser support. I frankly don't remember the exact context this came up in. If nothing else, this will prevent a genuine bug once we decide to allow length and size again for offset arrays. (Officially we said that would happen in 0.6, but I wonder if it's still a little early. Gotta finish JuliaLang/www_old.julialang.org#498 so they get more use...)
There was a problem hiding this comment.
Yup, it's the right definition, just checking if you were willing to incrementally begin allowing end expressions.
There was a problem hiding this comment.
Since I think end has an unambiguous meaning, I think it makes sense. Unless you see a problem I don't.
There was a problem hiding this comment.
any backporting concern that these don't have the @inline_meta on release-0.5 ?
There was a problem hiding this comment.
It should be fine to add them, I'd think.
There was a problem hiding this comment.
I would rather leave them out when it comes to backporting this specific PR. Looks like they were added in #20079 which is a larger breaking PR that's not a backport candidate. But if just this piece on its own has performance advantages and doesn't break anything, someone who wants it can propose it independently against release-0.5 (or at the moment, tk/backports-0.5.2)
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
Seems like the performance worked out fine. OK to merge? |
The
endofchange is worthy of @nanosoldierrunbenchmarks(ALL, vs = :master).