Slight rework of BitInt logic to allow larger integers

[Keno]

I was playing with using larger prime moduli using bit integers provided by the BitIntegers package. Everything just goes through, except that a couple function hardcoded which integers are bit integers. Fix those by only bailing out if we get to BigInt.

[tkluck]

Looks good! Let's add BitIntegers to the test dependencies and add tests for the expected behaviour?

[Keno]

The only reason this makes me hesitant to add to the tests is that I had to type pirate widen(::Type{Int128}) = Int256 to make things to really smooth and that's a global change obviously. If we want to test this, we should probably do it in a separate test process to make sure we test the default case also.

[codecov[bot]]

Codecov Report

Merging #6 into master will increase coverage by 0.28%. The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
+ Coverage   87.27%   87.55%   +0.28%     
==========================================
  Files          14       14              
  Lines         660      667       +7     
==========================================
+ Hits          576      584       +8     
+ Misses         84       83       -1

Impacted Files	Coverage Δ
src/BoundedIntegers.jl	63.41% <100%> (+3.89%)	:arrow_up:
src/Util.jl	75% <75%> (+2.27%)	:arrow_up:
src/PrimeFields.jl	89.33% <80%> (+0.44%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3e5ccb5...b61cebe. Read the comment docs.

[tkluck]

@Keno I just added a commit on top of yours that adds Requires.jl-based support for this. What do you think?

(I'm also trying to find out how test dependencies work in Project.toml. I cargo-culted what I saw elsewhere; Let's see what CI says.)

[Keno]

@Keno I just added a commit on top of yours that adds Requires.jl-based support for this. What do you think?

Seems fine to me, my only concern is that if we're adding it to the tests, we're no longer testing the non-BitIntegers path necessarily. The way we usually handle these kinds of situations is to split the tests with the optional dependency out into a separate file and launch them in a separate julia process, e.g. https://github.com/JuliaLang/julia/blob/master/test/boundscheck.jl#L10.

[Keno]

Just wanted to check if you were waiting on anything from me for this PR.

[tkluck]

@Keno no sorry, just me getting distracted. Will try not to make this linger too long.

[Keno]

No worries, just wanted to make sure I wasn't supposed to be doing something.

[tkluck]

Implemented & pushed your suggestion @Keno. There's this issue related to using Int256

ERROR: LoadError: MethodError: no method matching check_top_bit(::Int256)
Closest candidates are:
  check_top_bit(!Matched::Type{To}, !Matched::Any) where To at boot.jl:568
Stacktrace:
 [1] macro expansion at /home/tkluck/.julia/packages/BitIntegers/xlTFo/src/BitIntegers.jl:148 [inlined]
 [2] convertto(::Type{UInt256}, ::Int256) at /home/tkluck/.julia/packages/BitIntegers/xlTFo/src/BitIntegers.jl:148
 [3] UInt256(::Int256) at /home/tkluck/.julia/packages/BitIntegers/xlTFo/src/BitIntegers.jl:139
 [4] Unsigned(::Int256) at /home/tkluck/.julia/packages/BitIntegers/xlTFo/src/BitIntegers.jl:57
 [5] convert(::Type{Unsigned}, ::Int256) at ./number.jl:7
 [6] unsigned(::Int256) at /home/tkluck/.julia/packages/BitIntegers/xlTFo/src/BitIntegers.jl:134
 [7] ndigits0zpb(::Int256, ::Int64) at /home/tkluck/.julia/packages/BitIntegers/xlTFo/src/BitIntegers.jl:421
 [8] ndigits0z(::Int256, ::Int64) at ./intfuncs.jl:520
 [9] #ndigits#319(::Int64, ::Int64, ::typeof(ndigits), ::Int256) at ./intfuncs.jl:548
 [10] (::Base.var"#kw##ndigits")(::NamedTuple{(:base,),Tuple{Int64}}, ::typeof(ndigits), ::Int256) at ./intfuncs.jl:548
 [11] BigInt(::Int256) at ./gmp.jl:308
 [12] convert(::Type{BigInt}, ::Int256) at ./number.jl:7
 [13] big at ./gmp.jl:464 [inlined]
 [14] rem at /home/tkluck/.julia/packages/BitIntegers/xlTFo/src/BitIntegers.jl:354 [inlined]
 [15] rem(::Int256, ::Int64) at ./promotion.jl:353
 [16] isprime(::Int256) at /home/tkluck/.julia/packages/Primes/uaYlp/src/Primes.jl:145

so that's what I'd have to solve before merging. If you beat me to it, that's very welcome, of course :)

[Keno]

Update your copy of BitIntegers to master. This is an incompatibility between older BitIntegers and newer julia.

[tkluck]

That did the trick. Thanks again for the contribution!