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

Get rid of the tricks to avoid overflow in the AbstractUnitRange iterator #27147

Merged
merged 2 commits into from
May 19, 2018

Conversation

haampie
Copy link
Contributor

@haampie haampie commented May 18, 2018

Thanks to the new iterator protocol we can now iterate over a unit range in the number type of its elements, without having to worry about overflow :). Fixes #26586.

Previously iterating over say typemin(Int8) : typemax(Int8) was done by using an Int internally which was converted to an Int8 each iteration. Now we can just iterate with Int8.

Something similar can probably be done for StepRange, but that would be another pr.

@KristofferC
Copy link
Sponsor Member

KristofferC commented May 18, 2018

Just to check, there already exists tests for e.g. iterating

typemin(Int8) : typemax(Int8)

?

@haampie
Copy link
Contributor Author

haampie commented May 18, 2018

Yes: https://github.com/JuliaLang/julia/blob/master/test/ranges.jl#L421-L426 :)

@haampie
Copy link
Contributor Author

haampie commented May 18, 2018

Test failures seem unrelated (libgit, distributed), ranges tests are green everywhere :)

@StefanKarpinski
Copy link
Sponsor Member

Who would be good to review this?

@StefanKarpinski StefanKarpinski requested a review from Keno May 18, 2018 18:59
Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

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

Yep, that's how this is supposed to work :)

@Keno
Copy link
Member

Keno commented May 18, 2018

Let's run nanosoldier though, since LLVM is sensitive to how ranges are lowered:
@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

@Keno Keno merged commit 43a3886 into JuliaLang:master May 19, 2018
@haampie haampie deleted the fix-unitrange-iterator branch May 21, 2018 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants