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

fix digits(n::Unsigned) with neg base for n > typemax(n)÷2 #29187

Closed
wants to merge 3 commits into from
Closed

fix digits(n::Unsigned) with neg base for n > typemax(n)÷2 #29187

wants to merge 3 commits into from

Conversation

rfourquet
Copy link
Member

Fixes #29183. The same idea as in #29148 is used.

@rfourquet rfourquet added the bugfix This change fixes an existing bug label Sep 14, 2018
StefanKarpinski
StefanKarpinski reviewed Sep 14, 2018
View reviewed changes
base/intfuncs.jl Outdated
@@ -743,12 +743,25 @@ julia> digits!([2,2,2,2,2,2], 10, base = 2)
0
```
"""
function digits!(a::AbstractVector{T}, n::Integer; base::Integer = 10) where T<:Integer
base < 0 && isa(n, Unsigned) && return digits!(a, convert(Signed, n), base = base)
function digits!(a::AbstractVector{T}, n::Integer; base::Integer = 10, skipfirst=false) where T<:Integer
Copy link
Member

Choose a reason for hiding this comment

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

Should we call this __skipfirst__ or something to make it 100% clear that it's not part of the public API?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had only thought about _skipfirst ! and then didn't bother to re-push, as it's not in the docstring and it's not possible to use this keyword by accident (like it would be with a positional with default). But I realize now that i appears in the output of methods(digits!)... I guess I should rename.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gripe: if we had an in-place circshift!(a, 1), this keyword wouldn't be needed.

Copy link
Member

Choose a reason for hiding this comment

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

_skipfirst would be fine too. Depends on how many underscores you need to feel safe.

@StefanKarpinski
Copy link
Member

I've got a slightly different approach that avoids the skipfirst argument that I'll make a PR for.

StefanKarpinski added a commit that referenced this pull request Sep 15, 2018
@StefanKarpinski
fix digits(n::Unsigned) with neg base for n > typemax(n)÷2
0499528 
Alternative based on #29187
@StefanKarpinski StefanKarpinski mentioned this pull request Sep 15, 2018
Merged
StefanKarpinski added a commit that referenced this pull request Sep 15, 2018
@StefanKarpinski
fix digits(n::Unsigned) with neg base for n > typemax(n)÷2
731f384 
Alternative based on #29187
StefanKarpinski added a commit that referenced this pull request Sep 15, 2018
@StefanKarpinski
fix digits(n::Unsigned) with neg base for n > typemax(n)÷2
0a9bc41 
Alternative based on #29187
@rfourquet
Copy link
Member Author

different approach that avoids the skipfirst argument

Is it for not polluting the method signature? I was seeing that as a very minor inconvenience only.

@rfourquet
Copy link
Member Author

Also, in this version, I called digits recursively to avoid a a type instability like n = -signed(n) (or an ugly renaming nn = -signed(n). Otherwise, with a very minor change I can avoid the recursive call, cf this branch https://github.com/JuliaLang/julia/compare/rf/digits-unsigned-nonrecur?expand=1

StefanKarpinski added a commit that referenced this pull request Sep 16, 2018
@StefanKarpinski
fix digits(n::Unsigned) with neg base for n > typemax(n)÷2
4fb103a 
Alternative based on #29187

Tests from rforquet's PR linked above.
@StefanKarpinski
Copy link
Member

I started modifying this version slightly but it ended up being quite different. I find the non-recursive version that doesn't introduce any new arguments to digits! much easier to understand since it just peels a single loop iteration off of the loop in the negative base case and then continues as normal.

@rfourquet
Copy link
Member Author

Given that writing n = -signed(n) is not a problem, I removed the use of keyword. I guess the two PRs are now relatively similar.

@StefanKarpinski
Copy link
Member

The compiler may be able to figure out that it should hoist the base < 0 check out of the loop body (i.e. perform "loop unswitching") but I found that manually doing the loop unstitching made the code a bit simpler anyway. I think that a lot of the special case code for both ndigits and digits could potentially be unified into single implementations.

@rfourquet
Copy link
Member Author

I started modifying this version slightly but it ended up being quite different.

Yes no problem, I agree that the use of keyword in the last version was not the most beautiful expression of recursivity. I just updated this PR without keyword, but no problem to have this closed in favor of yours.

StefanKarpinski added a commit that referenced this pull request Sep 17, 2018
@StefanKarpinski
fix digits(n::Unsigned) with neg base for n > typemax(n)÷2
15ff960 
Alternative based on #29187

Tests from rforquet's PR linked above.
@StefanKarpinski
Copy link
Member

Merged #29205 instead.

StefanKarpinski added a commit that referenced this pull request Sep 17, 2018
@StefanKarpinski
fix digits(n::Unsigned) with neg base for n > typemax(n)÷2 (#29205)
16516b5 
Alternative based on #29187

Tests from rforquet's PR linked above.
KristofferC pushed a commit that referenced this pull request Sep 30, 2018
@StefanKarpinski @KristofferC
fix digits(n::Unsigned) with neg base for n > typemax(n)÷2 (#29205)
bba596f 
Alternative based on #29187

Tests from rforquet's PR linked above.

(cherry picked from commit 16516b5)
KristofferC pushed a commit that referenced this pull request Feb 11, 2019
@StefanKarpinski @KristofferC
fix digits(n::Unsigned) with neg base for n > typemax(n)÷2 (#29205)
ebadbd7 
Alternative based on #29187

Tests from rforquet's PR linked above.

(cherry picked from commit 16516b5)
KristofferC pushed a commit that referenced this pull request Feb 20, 2020
@StefanKarpinski @KristofferC
fix digits(n::Unsigned) with neg base for n > typemax(n)÷2 (#29205)
a5d9452 
Alternative based on #29187

Tests from rforquet's PR linked above.

(cherry picked from commit 16516b5)
@DilumAluthge DilumAluthge deleted the rf/digits-unsigned branch March 25, 2021 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StefanKarpinski StefanKarpinski StefanKarpinski left review comments

Assignees
No one assigned
Labels
bugfix This change fixes an existing bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rfourquet @StefanKarpinski