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.

Sign up for GitHub

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

More fixes for non-1 arrays #21251

Merged
merged 1 commit into from
Apr 4, 2017
Merged

More fixes for non-1 arrays #21251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion base/abstractarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, it's the right definition, just checking if you were willing to incrementally begin allowing end expressions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I think end has an unambiguous meaning, I think it makes sense. Unless you see a problem I don't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any backporting concern that these don't have the @inline_meta on release-0.5 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fine to add them, I'd think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine too.

first(a::AbstractArray) = a[first(eachindex(a))]

"""
Expand Down
18 changes: 11 additions & 7 deletions base/array.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1040,26 +1040,30 @@ function =={T<:BitInteger}(a::Array{T,1}, b::Array{T,1})
:memcmp, Int32, (Ptr{T}, Ptr{T}, UInt), a, b, sizeof(T) * len)
end

function reverse(A::AbstractVector, s=1, n=length(A))
function reverse(A::AbstractVector, s=first(linearindices(A)), n=last(linearindices(A)))
B = similar(A)
for i = 1:s-1
for i = first(linearindices(A)):s-1
B[i] = A[i]
end
for i = s:n
B[i] = A[n+s-i]
end
for i = n+1:length(A)
for i = n+1:last(linearindices(A))
B[i] = A[i]
end
return B
end
reverseind(a::AbstractVector, i::Integer) = length(a) + 1 - i
function reverseind(a::AbstractVector, i::Integer)
li = linearindices(a)
first(li) + last(li) - i
end

function reverse!(v::AbstractVector, s=1, n=length(v))
function reverse!(v::AbstractVector, s=first(linearindices(v)), n=last(linearindices(v)))
liv = linearindices(v)
if n <= s # empty case; ok
elseif !(1 ≤ s ≤ endof(v))
elseif !(first(liv) ≤ s ≤ last(liv))
throw(BoundsError(v, s))
elseif !(1 ≤ n ≤ endof(v))
elseif !(first(liv) ≤ n ≤ last(liv))
throw(BoundsError(v, n))
end
r = n
Expand Down
10 changes: 10 additions & 0 deletions test/offsetarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,8 +278,18 @@ am = map(identity, a)

# other functions
v = OffsetArray(v0, (-3,))
@test endof(v) == 1
@test v ≈ v
@test indices(v') === (Base.OneTo(1),-2:1)
rv = reverse(v)
@test indices(rv) == indices(v)
@test rv[1] == v[-2]
@test rv[0] == v[-1]
@test rv[-1] == v[0]
@test rv[-2] == v[1]
cv = copy(v)
@test reverse!(cv) == rv

A = OffsetArray(rand(4,4), (-3,5))
@test A ≈ A
@test indices(A') === (6:9, -2:1)
Expand Down