Fix overflow when initializing a Sieve with T::MAX

[dvdplm]

Avoids a panic when initializing a Sieve with T::MAX.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.33%. Comparing base (4a4696e) to head (20e06b3).
Report is 14 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #47   +/-   ##
=======================================
  Coverage   99.32%   99.33%           
=======================================
  Files           9        9           
  Lines        1340     1344    +4     
=======================================
+ Hits         1331     1335    +4     
  Misses          9        9           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fjarri]

Seems like here we're branching on something that will never happen unless the private attributes are messed with? Also, instead of returning None for a composite now the method returns None for a composite or if there's an overflow, which can be confusing.

[dvdplm]

This overflow can actually happen without any malicious shenanigans (and is guarded against with the sieve_with_max_start test).

[dvdplm]

I agree with you about the odd control flow here though. It gets complex quick:

• Create a sieve and set base to T::MAX (like the test).
• Call next()
• The first call to update_residues() will work fine because incr == 0 (returns true)
• …but this first call will set last_round to true as a side effect.
• Now we call maybe_next() which will also work fine because T::MAX is composite (returns None)
• …but now incr and incr_limit are 2 as a side effect.
• We call update_residues again, and given that incr is now 2 and less or equal to incr_limit, we match on the first check and return true (note: last_round is true, which would cause returning false)
• So now we call maybe_next again but now the addition overflows so we return None and the main loop continues
• Back in update_residues() for the third time, we finally hit the check for last_round, so we return false
• …which causes the main while-loop in next() to exit and we return None
• …and the iterator stops.

Pheew!!!

Maybe the Sieve constructor should check if the given start is worth the effort? E.g. check if T::MAX - start contains "enough" primes. Maybe using PNT and check that T::MAX/log(T::MAX) - start/log(start) is larger than some number (1?, 10?)? Or can you think of something smarter?

[dvdplm]

…then again, there are a lot of primes, so even if they pick a start that is 99.999999% of T::MAX there are still a LOT of prime candidates left, so maybe we just check that start != T::MAX and that's good enough?

[fjarri]

I don't particularly like the idea of heuristic checks and relying on probabilities in this specific case (the sieve), I think if it's possible to do exactly, we should try to do it.

Now we call maybe_next() which will also work fine because T::MAX is composite

Not necessarily, 2^128-1 and 2^8192-1 are primes. (Actually the former can be used as a test)

[dvdplm]

(Actually the former can be used as a test)

You mean 2^127 -1? Is that the last prime before U128::MAX, probably not right? Seems like a bit of an arbitrary test, or maybe I'm misunderstanding what you're suggesting here.

[fjarri]

Sorry, my mistake. Any 2^n-1 is indeed composite if n is composite.

[dvdplm]

@fjarri I think this one is ready to go in. I'd prefer to do any bigger refactoring of the sieve in a different PR (if we deem it's worth our time).