Check bounds for indexing tuples by logical masks#19737
Check bounds for indexing tuples by logical masks#19737tkelman merged 4 commits intoJuliaLang:masterfrom
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 |
tkelman
left a comment
tkelman
left a comment
There was a problem hiding this comment.
benchmark results are noise, lgtm
| 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.
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
deprecation
5 participants
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