Check bounds for indexing tuples by logical masks #19737
Conversation
Also deprecate indexing tuples by non-vectors since that should increase the dimensionality of the tuple, but tuples are 1-d only structures. Fix JuliaLang#19719
| getindex(t::Tuple, r::AbstractArray) = tuple([t[ri] for ri in r]...) | ||
| getindex(t::Tuple, b::AbstractArray{Bool}) = getindex(t,find(b)) | ||
| getindex{T}(t::Tuple, r::AbstractArray{T,1}) = tuple([t[ri] for ri in r]...) | ||
| getindex(t::Tuple, b::AbstractArray{Bool,1}) = length(b) == length(t) ? getindex(t,find(b)) : throw(BoundsError(t, b)) |
There was a problem hiding this comment.
apparently it's never been size-checked for as long as this getindex signature has existed 66ae77a, wonder if there was a reason not to
There was a problem hiding this comment.
Eh, I'd bet it was just an oversight. Bounds checking back then was less picky and no tests broke with this change.
|
just in case this might somehow be a hotspot? @nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
| @@ -15,8 +15,8 @@ endof(t::Tuple) = length(t) | |||
| size(t::Tuple, d) = d==1 ? length(t) : throw(ArgumentError("invalid tuple dimension $d")) | |||
| getindex(t::Tuple, i::Int) = getfield(t, i) | |||
| getindex(t::Tuple, i::Real) = getfield(t, convert(Int, i)) | |||
| getindex(t::Tuple, r::AbstractArray) = tuple([t[ri] for ri in r]...) | |||
| getindex(t::Tuple, b::AbstractArray{Bool}) = getindex(t,find(b)) | |||
| getindex{T}(t::Tuple, r::AbstractArray{T,1}) = tuple([t[ri] for ri in r]...) | |||
There was a problem hiding this comment.
r::AbstractVector ?
There was a problem hiding this comment.
It's not defined at this point in bootstrap.
There was a problem hiding this comment.
be sure to squash on merge then, if the first commit wouldn't build
|
Lots of PRs adding stuff at the end of |
Sacha0
added
kind:deprecation
Also deprecate indexing tuples by non-vectors since that should increase the
dimensionality of the tuple, but tuples are 1-d only structures.
Fix #19719