add length type parameter to StepRangeLen #41619
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
sostock
reviewed
Jul 17, 2021
82 tasks
| ref = start | ||
| step = (Δ/(len-1))*Δfac | ||
| else | ||
| imin = Int(len) | ||
| imin = len |
There was a problem hiding this comment.
The 2 and 1 on the call to steprangelen_hp below might also need their types adjusted.
timholy
reviewed
Jul 20, 2021
base/range.jl
Outdated
| 1 <= offset <= max(1,len) || throw(ArgumentError("StepRangeLen: offset must be in [1,$len], got $offset")) | ||
| new(ref, step, len, offset) | ||
| offset = convert(L, offset) | ||
| 1 <= offset <= max(1, len) || throw(ArgumentError("StepRangeLen: offset must be in [1,$len], got $offset")) |
There was a problem hiding this comment.
Maybe take the chance to put this in an internal function to avoid the GC frame? Saves a ns or so in the non-error case (~30%).
There was a problem hiding this comment.
I think I will leave that to someone else. This has grown already into a monstrosity (and I would like to see codegen do that transform automatically instead)
Also be more careful about using additive identity instead of multiplicative, and be more consistent about types in a few places. Fixes #41517
|
@JeffBezanson could you re-review? this now transforms LinRange too, for the sake of completeness |
Co-authored-by: Jeff Bezanson <[email protected]>
vtjnash
added
the
status:merge me
vtjnash
removed
the
status:merge me
Closed
| # For #18336 we need to prevent promotion of the step type: | ||
| broadcasted(::DefaultArrayStyle{1}, ::typeof(+), r::AbstractRange, x::Number) = range(first(r) + x, step=step(r), length=length(r)) | ||
| broadcasted(::DefaultArrayStyle{1}, ::typeof(+), x::Number, r::AbstractRange) = range(x + first(r), step=step(r), length=length(r)) | ||
| broadcasted(::DefaultArrayStyle{1}, ::typeof(+), r::OrdinalRange, x::Real) = range(first(r) + x, last(r) + x, step=step(r)) | ||
| broadcasted(::DefaultArrayStyle{1}, ::typeof(+), x::Real, r::Real) = range(x + first(r), x + last(r), step=step(r)) |
There was a problem hiding this comment.
This line should supposedly have r::OrdinalRange, but cf #42291.
KristofferC
pushed a commit
that referenced
this pull request
Jul 8, 2022
* Document that `L`ength parameter needs Julia 1.7 vtjnash added the 4th type parameter `L` in #41619 to `StepRangeLen`. That addition seems to have appeared in Julia 1.7 so it should be documented as such. I found this out the hard way because my CI for code that used `L` is failing with Julia 1.6. Co-authored-by: Johnny Chen <[email protected]> Co-authored-by: Keno Fischer <[email protected]>
ffucci
pushed a commit
to ffucci/julia
that referenced
this pull request
Aug 11, 2022
* Document that `L`ength parameter needs Julia 1.7 vtjnash added the 4th type parameter `L` in JuliaLang#41619 to `StepRangeLen`. That addition seems to have appeared in Julia 1.7 so it should be documented as such. I found this out the hard way because my CI for code that used `L` is failing with Julia 1.6. Co-authored-by: Johnny Chen <[email protected]> Co-authored-by: Keno Fischer <[email protected]>
pcjentsch
pushed a commit
to pcjentsch/julia
that referenced
this pull request
Aug 18, 2022
* Document that `L`ength parameter needs Julia 1.7 vtjnash added the 4th type parameter `L` in JuliaLang#41619 to `StepRangeLen`. That addition seems to have appeared in Julia 1.7 so it should be documented as such. I found this out the hard way because my CI for code that used `L` is failing with Julia 1.6. Co-authored-by: Johnny Chen <[email protected]> Co-authored-by: Keno Fischer <[email protected]>
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.
Also be more careful about using additive identity instead of
multiplicative, and be more consistent about types in a few places.
Fixes #41517