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

ndigits: check for invalid bases (fix #16766) #16841

Merged
merged 3 commits into from
May 12, 2017
Merged

ndigits: check for invalid bases (fix #16766) #16841

merged 3 commits into from
May 12, 2017

Conversation

rfourquet
Copy link
Member

I made ndigits(x, b) with -1 <= b <= 1 an error in all cases, even if x == 0, as I thought it was more consistent. There were a lot of dispatch methods ndigits[0z][nb](x, [b]), so it's possible I messed up one of the paths.
Also I removed the ndigits(::BigInt, [b]) method, to handle the base check in only one place, the drawback being that we then compare x == 0 instead of the probably more efficient x.size == 0.
Hopefully this is overall a simplification of the ndigits logic.

tkelman
tkelman reviewed Jun 9, 2016
View reviewed changes
base/intfuncs.jl Outdated

ndigits(x::Unsigned, b::Integer) = x==0 ? 1 : ndigits0z(x,Int(b))
ndigits(x::Unsigned) = x==0 ? 1 : ndigits0z(x)
ndigits(x::Integer, b::Integer) =
Copy link
Contributor

Choose a reason for hiding this comment

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

use the function form here

@rfourquet
Copy link
Member Author

Thanks for reviewing @tkelman, comments addressed.

@tkelman
Copy link
Contributor

tkelman commented Jun 9, 2016

LGTM! The added comments are quite useful, thanks for those.

@rfourquet
Copy link
Member Author

I must admit that the "0z" suffix is not 100% clear for me, hence left uncommented!

@StefanKarpinski
Copy link
Member

If keyword arguments didn't introduce too much overhead still, we could make the signature

digits(n; base::Integer=10, min::Integer=1)

That way you could do digits(n, min=0) instead of digits0z.

@rfourquet
Copy link
Member Author

Do we agree on the idea that: ndigits(x, b) with -1 <= b <= 1 is an error in all cases, even if x == 0?
Does the z in ndigits0z stands for zero? I read that as "same as ndigits but gives zero on input 0". If this is guaranteed, it can be a useful function in user-code (e.g. here). I modified BigInt's version accordingly, but I could add some tests and doc.

@StefanKarpinski
Copy link
Member

Does the z in ndigits0z stands for zero? I read that as "same as ndigits but gives zero on input 0".

Yes, that's the correct interpretation.

@rfourquet
Copy link
Member Author

I pushed a new commit which renames ndigits0z to ndigits0zpb and makes ndigits0z(n, b) = ndigits(n, b, min1=true) a very thin wrapper over ndigits (min1 is a positional argument here), which could be removed (in line with what Stefan suggested above). Makes sense?

@tkelman
Copy link
Contributor

tkelman commented Aug 11, 2016

Sorry this got neglected, could you rebase?

@rfourquet
Copy link
Member Author

Updated, but please give me a couple more days before merging, I would like to squash and review once more (I can't continue working on it right now).

@rfourquet
Copy link
Member Author

Rebased, and I replaced my min1 implementation mentioned above with a pad positional argument, in line with what Stefan had suggested above (not documented, should probably become a keyword arg eventually). I'm fine with renaming it to min or with removing it altogether.

@tkelman
Copy link
Contributor

tkelman commented Aug 27, 2016

Great, LGTM. Anyone else have any thoughts?

@StefanKarpinski
Copy link
Member

Let's get some votes on pad versus min:

@StefanKarpinski
Copy link
Member

pad

@StefanKarpinski
Copy link
Member

min

@kshyatt
Copy link
Contributor

kshyatt commented Sep 7, 2016

Needs a rebase.

@rfourquet
Copy link
Member Author

Should I wait till 0.6 is released before rebasing and hopefully merging?

@StefanKarpinski
Copy link
Member

Yes, I'm afraid that since this includes user-visible changes, we'll have to wait at this point, so no point in rebasing before we branch release-0.6 from master.

@rfourquet rfourquet force-pushed the rf/ndigits-bases branch 3 times, most recently from 15c6da9 to 1b7f40f Compare May 3, 2017 14:21
@rfourquet
Copy link
Member Author

I rebased now, the travis errors are apparently independant. This is good to go on my side.

tkelman
tkelman reviewed May 3, 2017
View reviewed changes
base/intfuncs.jl Outdated
ndigits0zpb(x::Base.BitSigned, b::Integer) = ndigits0zpb(unsigned(abs(x)), Int(b))
ndigits0zpb(x::Base.BitUnsigned, b::Integer) = ndigits0zpb(x, Int(b))

# The suffix "0z" means that the ouput is 0 on input zero (cf. #16841)
Copy link
Contributor

Choose a reason for hiding this comment

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

output

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@rfourquet rfourquet force-pushed the rf/ndigits-bases branch from 1b7f40f to 65002d7 Compare May 3, 2017 18:12
@rfourquet rfourquet mentioned this pull request May 3, 2017
Merged
stevengj
stevengj reviewed May 3, 2017
View reviewed changes
base/intfuncs.jl Outdated

# TODO (when keywords args are fast): rename to ndigits and make pad a keyword
ndigits10(x::Integer, pad::Int=1) = max(pad, ndigits0z(x))
ndigits(x::Integer) = x == 0 ? 1 : ndigits0z(x)
Copy link
Member

Choose a reason for hiding this comment

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

iszero(x) would be more efficient for BigInt, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thank you.

@rfourquet
Copy link
Member Author

I will merge in a couple of days if no objections.

@rfourquet rfourquet merged commit 75ca24f into JuliaLang:master May 12, 2017
@rfourquet rfourquet deleted the rf/ndigits-bases branch May 12, 2017 17:17
@TotalVerb TotalVerb mentioned this pull request May 17, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman left review comments

@stevengj stevengj stevengj left review comments

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

Successfully merging this pull request may close these issues.
5 participants
@rfourquet @tkelman @StefanKarpinski @kshyatt @stevengj