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.

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

checkbounds return value #13334

Closed
garrison opened this issue Sep 27, 2015 · 4 comments
Closed

checkbounds return value #13334

garrison opened this issue Sep 27, 2015 · 4 comments
Labels
docs This change adds or pertains to documentation error handling Handling of exceptions by Julia or the user help wanted Indicates that a maintainer wants help on an issue or pull request

Comments

@garrison
Copy link
Sponsor Member

I was a bit surprised to notice that the function checkbounds(A::AbstractArray{T,N}, I...) returns true if it does not throw a BoundsError.

julia> checkbounds(1:3, 4)
ERROR: BoundsError: attempt to access 1:3
  at index [4]

julia> checkbounds(1:3, 3)
true

I would have expected it to return nothing, especially since the documentation makes no mention of return value:

Throw an error if the specified indexes are not in bounds for the given array. Subtypes of AbstractArray should specialize this method if they need to provide custom bounds checking behaviors.

Note that there is another version of checkbounds that is explicitly asked to return a Bool:

julia> checkbounds(Bool, 3, 4)
false

julia> checkbounds(Bool, 3, 3)
true

Two possible solutions:

  1. change checkbounds(A::AbstractArray{T,N}, I...) to return nothing if it does not throw an error. This will involve going through existing packages to make sure nobody is relying on the current behavior.
  2. decide that returning true is actually no accident, and modify the documentation to mention the return value. If this decision is taken, it's especially important to make this explicit in the documentation, as subtypes of AbstractArray will want to also have their checkbounds return true to be consistent with the original interface (otherwise there can be subtle breakage).

My vote is for the first option, as returning true is redundant if no error is thrown.

@kshyatt kshyatt added docs This change adds or pertains to documentation needs decision A decision on this change is needed error handling Handling of exceptions by Julia or the user labels Nov 25, 2015
@kshyatt
Copy link
Contributor

kshyatt commented Nov 25, 2015

Is this still a problem? What would we like to do?

@timholy
Copy link
Sponsor Member

timholy commented Jun 25, 2016

👍 on your proposal. I'm going to strip out the "decision" tag here, and mark "up for grabs."

@timholy timholy added help wanted Indicates that a maintainer wants help on an issue or pull request and removed needs decision A decision on this change is needed labels Jun 25, 2016
@Tetralux
Copy link
Contributor

This appears to have been fixed. (Checked on commit 6a1483b.)

julia> checkbounds(1:3, 4)
ERROR: BoundsError: attempt to access 3-element UnitRange{Int64} at index [4]
 in throw_boundserror(::UnitRange{Int64}, ::Tuple{Int64}) at .\abstractarray.jl:326
 in checkbounds(::UnitRange{Int64}, ::Int64) at .\abstractarray.jl:283

julia> checkbounds(1:3, 3)

julia>

@mbauman
Copy link
Sponsor Member

mbauman commented Oct 11, 2016

Yup. Fixed by the revamp in #17355.

@mbauman mbauman closed this as completed Oct 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation error handling Handling of exceptions by Julia or the user help wanted Indicates that a maintainer wants help on an issue or pull request
Projects
None yet
Development

No branches or pull requests

5 participants