Skip to content

Commit

Permalink
Eagerly do boundscheck when indexing CartesianIndices with CartesianI…
Browse files Browse the repository at this point in the history
…ndices (#42235)
  • Loading branch information
johnnychen94 committed Feb 17, 2022
1 parent 2aaa25a commit 7a1c20e
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 3 deletions.
11 changes: 8 additions & 3 deletions base/multidimensional.jl
Original file line number Diff line number Diff line change
Expand Up @@ -355,14 +355,19 @@ module IteratorsMD
# CartesianIndices act as a multidimensional range, so cartesian indexing of CartesianIndices
# with compatible dimensions may be seen as indexing into the component ranges.
# This may use the special indexing behavior implemented for ranges to return another CartesianIndices
@propagate_inbounds function Base.getindex(iter::CartesianIndices{N,R},
@inline function Base.getindex(iter::CartesianIndices{N,R},
I::Vararg{Union{OrdinalRange{<:Integer, <:Integer}, Colon}, N}) where {N,R}
CartesianIndices(getindex.(iter.indices, I))
@boundscheck checkbounds(iter, I...)
indices = map(iter.indices, I) do r, i
@inbounds getindex(r, i)
end
CartesianIndices(indices)
end
@propagate_inbounds function Base.getindex(iter::CartesianIndices{N},
C::CartesianIndices{N}) where {N}
CartesianIndices(getindex.(iter.indices, C.indices))
getindex(iter, C.indices...)
end
@inline Base.getindex(iter::CartesianIndices{0}, ::CartesianIndices{0}) = iter

# If dimensions permit, we may index into a CartesianIndices directly instead of constructing a SubArray wrapper
@propagate_inbounds function Base.view(c::CartesianIndices{N}, r::Vararg{Union{OrdinalRange{<:Integer, <:Integer}, Colon},N}) where {N}
Expand Down
9 changes: 9 additions & 0 deletions test/boundscheck_exec.jl
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ if bc_opt == bc_default || bc_opt == bc_off
end

@testset "pass inbounds meta to getindex on CartesianIndices (#42115)" begin
@inline getindex_42115(r, i) = @inbounds getindex(r, i)
@inline getindex_42115(r, i, j) = @inbounds getindex(r, i, j)

R = CartesianIndices((5, 5))
Expand All @@ -270,6 +271,14 @@ end
@test getindex_42115(R, -1, -1) == CartesianIndex(-1, -1)
@test getindex_42115(R, 1, -1) == CartesianIndex(1, -1)
end

if bc_opt == bc_on
@test_throws BoundsError getindex_42115(R, CartesianIndices((6, 6)))
@test_throws BoundsError getindex_42115(R, -1:3, :)
else
@test getindex_42115(R, CartesianIndices((6, 6))) == CartesianIndices((6, 6))
@test getindex_42115(R, -1:3, :) == CartesianIndices((-1:3, 1:5))
end
end


Expand Down

4 comments on commit 7a1c20e

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

Please sign in to comment.