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

mod(n, range) for integers #32628

Merged
merged 7 commits into from
Jul 24, 2019
Merged

mod(n, range) for integers #32628

merged 7 commits into from
Jul 24, 2019

Conversation

mcabbott
Copy link
Contributor

Implements #32571.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jul 20, 2019 •
edited
Loading

Ok, this looks great but there are some test failures which of course need to be dealt with. It seems like a simple one: OneTo is not available in the test code so you need to either import it or qualify it.

@mcabbott
Copy link
Contributor Author

mcabbott commented Jul 22, 2019 •
edited
Loading

Sorry about the OneTo problem, should be fixed. Now there are buildbot errors which I don't understand, are they my fault?

I also just realised this originally allowed some ranges which it shouldn't:

julia> mod(5, UnitRange(1.2, 3.2))
1.9999999999999998

So I have restricted it to r::AbstractUnitRange{<:Integer}.

And I have also added a docstring, and a line to NEWS.md. Hope that's not excessive.

@StefanKarpinski StefanKarpinski requested a review from mbauman July 22, 2019 20:10
mbauman
mbauman reviewed Jul 23, 2019
View reviewed changes
Copy link
Member

@mbauman mbauman left a comment

Choose a reason for hiding this comment

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

This is cool — yet another nice case where removing implicit vecorization has allowed us to do something better. 👍

NEWS.md Outdated Show resolved Hide resolved
base/range.jl Outdated Show resolved Hide resolved
base/range.jl Outdated Show resolved Hide resolved
mcabbott and others added 3 commits July 24, 2019 08:35
@mcabbott
step +1 only
85ad76a
@mcabbott @mbauman
more detailed NEWS.md
f71867e 
Co-Authored-By: Matt Bauman <mbauman@gmail.com>
@mcabbott
docstring++
f93604a
@mbauman
Copy link
Member

mbauman commented Jul 24, 2019

The test failures look unrelated. Thanks!

@mbauman mbauman merged commit 2182389 into JuliaLang:master Jul 24, 2019
This was referenced Jul 24, 2019
Closed
Closed
mbauman added a commit that referenced this pull request Jul 24, 2019
@mbauman
Add compat note for mod(n, range)
4a672a4 
Followup to #32628
@mbauman mbauman mentioned this pull request Jul 24, 2019
Merged
@mcabbott mcabbott deleted the mod branch July 24, 2019 14:04
fredrikekre pushed a commit that referenced this pull request Jul 28, 2019
@mbauman @fredrikekre
Add compat note for mod(n, range), followup to #32628 (#32669)
a811842
mcabbott added a commit to mcabbott/Compat.jl that referenced this pull request Sep 26, 2019
@mcabbott
https://github.com/JuliaLang/julia/pull/32628
3084d03
martinholters pushed a commit to JuliaLang/Compat.jl that referenced this pull request Sep 29, 2019
@mcabbott @martinholters
mod(n, range) (#661)
33b906f 
* JuliaLang/julia#32628
@Keno Keno mentioned this pull request Feb 28, 2020
Closed
@jyjemily jyjemily mentioned this pull request May 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbauman mbauman mbauman left review comments

@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.
3 participants
@mcabbott @StefanKarpinski @mbauman