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.

Sign up for GitHub

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

Jump to bottom

Disallow ranges with Integer eltype but non-integer step #32439

Merged
merged 14 commits into from
Nov 30, 2020
Merged

Disallow ranges with Integer eltype but non-integer step #32439

merged 14 commits into from
Nov 30, 2020

Conversation

sostock
Copy link
Contributor

@sostock sostock commented Jun 27, 2019 •
edited
Loading

Fixes #32419.

@sostock sostock changed the title Disallow StepRange{<:Integer} with non-integer step Disallow StepRange{<:Integer} and StepRangeLen{<:Integer} with non-integer step Oct 17, 2019
@sostock
Copy link
Contributor Author

sostock commented Oct 17, 2019 •
edited
Loading

I now added the same check for StepRangeLen, since it had the same problem.

Before:

julia> StepRangeLen{Int}(1, 1//2, 5);

julia> collect(ans)
ERROR: InexactError: Int64(3//2)

After:

julia> StepRangeLen{Int}(1, 1//2, 5);
ERROR: ArgumentError: StepRangeLen{<:Integer} cannot have non-integer step

@sostock sostock mentioned this pull request Mar 23, 2020
Closed
@sostock
Copy link
Contributor Author

sostock commented Mar 23, 2020

I added the same check for LinRange{<:Integer}.

@sostock sostock changed the title Disallow StepRange{<:Integer} and StepRangeLen{<:Integer} with non-integer step Disallow ranges with Integer eltype but non-integer step Mar 23, 2020
@sostock
Copy link
Contributor Author

sostock commented Apr 30, 2020

Can this be in 1.5?

timholy
timholy approved these changes Apr 30, 2020
View reviewed changes
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check one item and then merge. Nice & thorough, thanks!

test/ranges.jl Outdated Show resolved Hide resolved
@timholy
Copy link
Member

timholy commented Apr 30, 2020

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@sostock
Copy link
Contributor Author

sostock commented Aug 17, 2020

Can this be in 1.6?

@StefanKarpinski
Copy link
Member

@timholy this good to go from your POV?

@StefanKarpinski StefanKarpinski merged commit c31d126 into JuliaLang:master Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timholy timholy timholy approved these changes

@StefanKarpinski StefanKarpinski StefanKarpinski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

StepRange{<:Integer} with non-integer step
4 participants
@sostock @timholy @nanosoldier @StefanKarpinski