-
-
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
docstring setindex #32738
docstring setindex #32738
Conversation
base/tuple.jl
Outdated
setindex(c::Tuple, v, i::Integer) | ||
|
||
Creates a new tuple similar to `x` with the value at index `i` set to `v`. | ||
An out-of-bound `i` makes it a no-op. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that intentional? Seems like odd behavior. I would assume it should boundscheck by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea, @vtjnash was the last to edit the line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was introduced in #20154 as a simple helper function. The behavior is very much intentional there, but perhaps we should see if we can add a @boundscheck
block without sacrificing performance if we're going to move towards this being more documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be considered as a breaking change then (replacing noop with BoundsError)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really as it was neither exported nor documented. That's why if we document it, it'd be good to make sure the semantics are what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI StaticArrays.jl does bound check with setindex
https://github.com/JuliaArrays/StaticArrays.jl/blob/2583967e282b21fd8f8a4e2fc306efc672b4d83d/src/deque.jl#L73-L86
I opened two issues in ArrayInterface.jl. But maybe this should be discussed in Base? |
First take on @mbauman's idea: It seems slightly slower, but not all the time (which is curious) |
@tkf for the moment only |
up here, I updated the gist above, there seems to be some significant performance hits when |
In any case, |
bump here, @mbauman? |
I don't think we should document it to be a no-op for out of bounds accesses. I think we should change the behavior to check bounds with |
Thanks, I'll make the commits following the gist above |
no idea why the tests are failing here, it seems unrelated to setindex |
No, I believe that's very much related. Looks like we've been relying on the ignore-out-of-bounds behavior in some internal implementations. |
Sorry yes, it's a bit weird to have part of the CI running fine. @inline selectdim(A::AbstractArray, d::Integer, i) = _selectdim(A, d, i, setindex(map(Slice, axes(A)), i, d))
@noinline function _selectdim(A, d, i, idxs)
d >= 1 || throw(ArgumentError("dimension must be ≥ 1, got $d"))
nd = ndims(A)
d > nd && (i == 1 || throw(BoundsError(A, (ntuple(k->Colon(),d-1)..., i))))
return view(A, idxs...)
end The good news is this should be fixable without breaking, but could lead to performance regression. Since we are errorring in |
I'd just copy the old implementation and call it something internally like As far as why ~half the CI services report ✅ and the other half report ❌, it's because we've split the process in two parts. The "package" buildbots just build Julia whereas the "test" buildbots are the ones that actually run the test suite. There are other buildbots that specifically run a handful of other checks. |
The other possibility is using the intern |
Any reason not to just merge this once CI passes? |
this is changing some behavior if somebody was using the no-op as a feature |
I say we merge it — it's a (formerly) undocumented internal function, so if folks were relying on this somewhat obscure functionality it's on them. |
Thanks so much for the patience and endurance in pushing this one through, @matbesancon! |
Thanks for the help along the way, my pleasure :) |
Even though it's not exported, it is nice to have the semantics available in a docstring, to use it elsewhere (see https://github.com/JuliaDiffEq/ArrayInterface.jl/pull/15/files)