deprecate unsafe_length for length #40382
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vtjnash
force-pushed
the
jn/unsafe_length
branch
from
3cc9cba to
6c549a9
Compare
|
Very nice to eliminate these ugly functions. Triage is concerned about what checks we should have though. We discussed checking on construction whether a range will have a correct length, and moving the error there. |
vtjnash
force-pushed
the
jn/unsafe_length
branch
from
6c549a9 to
9a7bea2
Compare
|
triage thought this was a reasonable change, but that we should keep the current version around as |
fredrikekre
reviewed
May 27, 2021
vtjnash
added
needs docs
This seems to be a fairly arbitrary case for throwing exceptions, when the user might often use this value in arithmetic afterwards, which is not checked. It leads to awkward complexity in the API however, where it may be unclear which function to reach for, with no particular justification for why a particular usage is "safe". And it inhibits optimization and performance due to the additional checks and error cases (and is not even entirely type-stable).
|
@JeffBezanson this should be ready to go now. It removes checking for overflow in length for |
JeffBezanson
approved these changes
Jun 21, 2021
JeffBezanson
changed the title
DilumAluthge
removed
the
merge me
johanmon
pushed a commit
to johanmon/julia
that referenced
this pull request
Jul 5, 2021
This seems to be a fairly arbitrary case for throwing exceptions, when the user might often use this value in arithmetic afterwards, which is not checked. It leads to awkward complexity in the API however, where it may be unclear which function to reach for, with no particular justification for why a particular usage is "safe". And it inhibits optimization and performance due to the additional checks and error cases (and is not even entirely type-stable).
|
After this pull request julia> using Unitful
julia> length(1u"m":2u"m":0u"m")
0 m |
KristofferC
pushed a commit
that referenced
this pull request
Jul 26, 2021
This seems to be a fairly arbitrary case for throwing exceptions, when the user might often use this value in arithmetic afterwards, which is not checked. It leads to awkward complexity in the API however, where it may be unclear which function to reach for, with no particular justification for why a particular usage is "safe". And it inhibits optimization and performance due to the additional checks and error cases (and is not even entirely type-stable). (cherry picked from commit 3eefaf0)
82 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 15, 2023
I noticed [here](#50467 (comment)) that `lastindex(x::Base.OneTo)` is not simply `x.stop`. This PR performs that optimization and many more by assuming `size` always returns positive numbers. ``` julia> @code_native lastindex(Base.OneTo(5)) # master .section __TEXT,__text,regular,pure_instructions .build_version macos, 13, 0 .globl _julia_lastindex_81 ; -- Begin function julia_lastindex_81 .p2align 2 _julia_lastindex_81: ; @julia_lastindex_81 ; ┌ @ abstractarray.jl:423 within `lastindex` ; %bb.0: ; %top ; │┌ @ abstractarray.jl:386 within `eachindex` ; ││┌ @ abstractarray.jl:134 within `axes1` ; │││┌ @ range.jl:708 within `axes` ; ││││┌ @ range.jl:471 within `oneto` ; │││││┌ @ range.jl:469 within `OneTo` @ range.jl:454 ; ││││││┌ @ promotion.jl:532 within `max` ; │││││││┌ @ int.jl:83 within `<` ldr x8, [x0] ; │││││││└ ; │││││││┌ @ essentials.jl:642 within `ifelse` cmp x8, #0 csel x0, x8, xzr, gt ; │└└└└└└└ ret ; └ ; -- End function .subsections_via_symbols julia> @code_native lastindex(Base.OneTo(5)) # pr .section __TEXT,__text,regular,pure_instructions .build_version macos, 13, 0 .globl _julia_lastindex_13253 ; -- Begin function julia_lastindex_13253 .p2align 2 _julia_lastindex_13253: ; @julia_lastindex_13253 ; ┌ @ abstractarray.jl:423 within `lastindex` ; %bb.0: ; %top ldr x0, [x0] ret ; └ ; -- End function .subsections_via_symbols ``` Also removed `axes(r::AbstractRange) = (oneto(length(r)),)` (added in #40382, @vtjnash) as redundant with the general `axes` method. The obvious downside here is that if someone defines an object with negative size, its axes will include Base.OneTo with negative stop. I think that is acceptable, but if not, we can gate this optimization to a set of known types (all AbstractArray types defined in Base should have non-negative size)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This seems to be a fairly arbitrary case for throwing exceptions, when the user might often use this value in arithmetic afterwards (e.g. see
searchsorted), which is not checked. It leads to awkward complexity in the API however, where it may be unclear which function to reach for, with no particular justification for why a particular usage is "safe". And it may inhibit optimization and performance due to the additional checks and error cases (and is not even entirely type-stable).Performance seems to be basically identical: