-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
Policy regarding @inbounds annotations #20469
Comments
The only problem here is that
This means that custom array types are safe… unless they depend upon julia> immutable BadVector{T} <: AbstractVector{Int}
data::T
end
Base.size(X::BadVector) = size(X.data)
Base.getindex(X::BadVector, i::Int) = X.data[i-1]
julia> BadVector(1:3)[:] # Nonscalar indexing is implemented with `@inbounds` and this is safe!
ERROR: BoundsError: attempt to access 3-element UnitRange{Int64} at index [0]
Stacktrace:
[1] throw_boundserror(::UnitRange{Int64}, ::Int64) at ./abstractarray.jl:365
[2] getindex at ./range.jl:512 [inlined]
[3] getindex(::BadVector{UnitRange{Int64}}, ::Int64) at ./REPL[12]:5
[4] macro expansion at ./multidimensional.jl:414 [inlined]
[5] macro expansion at ./cartesian.jl:62 [inlined]
[6] macro expansion at ./multidimensional.jl:411 [inlined]
[7] _unsafe_getindex! at ./multidimensional.jl:406 [inlined]
[8] macro expansion at ./multidimensional.jl:400 [inlined]
[9] _unsafe_getindex(::Base.LinearSlow, ::BadVector{UnitRange{Int64}}, ::Base.Slice{Base.OneTo{Int64}}) at ./multidimensional.jl:393
[10] macro expansion at ./multidimensional.jl:382 [inlined]
[11] _getindex at ./multidimensional.jl:378 [inlined]
[12] getindex(::BadVector{UnitRange{Int64}}, ::Colon) at ./abstractarray.jl:793
julia> BadVector([1,2,3])[:] # But this isn't because it propagates too far
3-element Array{Int64,1}:
7955998441852384046
1
2 |
Thanks. So at least we know where we're heading to. Then I'd say policy should anticipate on this, and we should use |
I've been meaning to ask, since I've been writing some Sometimes I'd like to be able to remove multiple checks of array sizes. For example, (And yes, for e.g. 3x3 matrix multiplication, the overhead of unnecessary things in |
So it looks like this just needs #20485 and maybe a review of the verbiage in the |
Ah, I missed that this issue |
OK, thanks. So that means for #21402 I only need to add |
(edit, just realized this issue was closed already!) In #27384 I looked at the example of For iterators in general, my guess is that almost always iteration can be guaranteed to go inbounds by the iterator itself*, at least for concrete implementations. That way we avoid placing awkward * there's obviously no guarantee for (1) The iteration state of My hope is that with the above policy, we could implement the function iterate(A::AbstractArray, state=(eachindex(A),))
y = iterate(state...)
y === nothing && return nothing
(@inbounds A[y[1]]), (state[1], tail(y)...)
end so that we could limit the |
This issue was raised at #20421 (comment). We should define clear rules regarding when it's OK to use
@inbounds
in Base code. Currently the code seems to be inconsistent.@JeffBezanson mentioned this: "only use
@inbounds
when you can be certain, from local information, that all accesses are in bounds." In a strict interpretation, that would mean@inbounds for i in eachindex(a)
is not correct whena::AbstractArray
, since an incorrect array implementation could return invalid indices, which would crash Julia.This strict interpretation is problematic since (as @stevengj noted) often a generic
AbstractArray
method is also used forArray
: if we cannot use@inbounds
there, performance suffers in the common case just to avoid possible crashes in rare cases. A mechanism to enable@inbounds
only for trusted types could help, but it would still be too bad that custom array types wouldn't benefit from bounds checking removal even when they implemented everything correctly: that would defeat the goal of making user-defined type as efficient as Base types. So I would say we need to trust custom array types, or at least allow them to opt out of bounds checking by stating that they are safe. Cf. #15291.The text was updated successfully, but these errors were encountered: