JDK 9 RFR of 8026236: Add PrimeTest for BigInteger [TEST-ONLY]

[Joe Darcy]

Hi Brian,

I've filed a follow-up bug:

         JDK-8042478: Include Mersenne primes in BigInteger primality 
testing

Cheers,

-Joe

On 5/5/2014 2:17 PM, Brian Burkhalter wrote:
> Joe / Paul,
>
> Thanks for the comments. This an aggregate response to your two 
> messages. An updated version of the patch is here:
>
> http://cr.openjdk.java.net/~bpb/8026236/webrev.03/ 
> <http://cr.openjdk.java.net/%7Ebpb/8026236/webrev.03/>
>
> I believe that it addresses all the points of concern aside from 
> testing Mersenne primes.
>
> Please see more comments in line.
>
> Thanks,
>
> Brian
>
> On May 3, 2014, at 9:55 AM, Joe Darcy <joe.darcy at oracle.com 
> <mailto:joe.darcy at oracle.com>> wrote:
>
>> Hi Brian,
>>
>> I think the parsePrimes method would be better with a different name 
>> since no parsing is occurring anymore.
>
> Renamed methods to createPrimes() and getPrimes().
>
>> I think someone in this test the fact that Integer.MAX_VALUE is a 
>> prime should be mentioned in taken advantage of :-)
>
> Done. Simply appended this one value to the list. ;-)
>
>> How about adding another test method which tests some Mersenne primes 
>> for primality? [1]
>
> No done.
>
>> I'd prefer to see some comments on the primes method briefly 
>> explaining it methodology.
>
> Done via a link to Wikipedia.
>
>> Would the running time be unacceptable (or memory usage too large) if 
>> the limit were set to Integer.MAX_VALUE?
>
> Yes, I tried it. Still running after at least ten minutes when I 
> killed it.
>
>> Thanks,
>>
>> -Joe
>>
>> [1]http://en.wikipedia.org/wiki/Mersenne_prime
>
> On May 5, 2014, at 2:11 AM, Paul Sandoz <paul.sandoz at oracle.com 
> <mailto:paul.sandoz at oracle.com>> wrote:
>
>> On May 3, 2014, at 6:55 PM, Joe Darcy <joe.darcy at oracle.com 
>> <mailto:joe.darcy at oracle.com>> wrote:
>>
>>> Hi Brian,
>>>
>>> I think the parsePrimes method would be better with a different name 
>>> since no parsing is occurring anymore.
>>>
>>
>> Yes, and there is no need for the try block.
>
> Removed.
>
>> (The use of BitSet.stream() also reminds me we can implement a 
>> spliterator with size estimates based on the words in use, currently 
>> the stream() is created from an iterator.)
>
> Made no change based on this comment (which I interpreted as an aside).
>
>>
>>> I think someone in this test the fact that Integer.MAX_VALUE is a 
>>> prime should be mentioned in taken advantage of :-)  How about 
>>> adding another test method which tests some Mersenne primes for 
>>> primality? [1]
>>>
>>> I'd prefer to see some comments on the primes method briefly 
>>> explaining it methodology. Would the running time be unacceptable 
>>> (or memory usage too large) if the limit were set to Integer.MAX_VALUE?
>>>
>>
>> +1 to both of those, although perhaps the former could be done as a 
>> second iteration to get this code in?
>
> This is what I did: +1 on the both but not the Mersennes.
>
>> Also, probably better to create the "NavigableSet<BigInteger> primes" 
>> just once in "main", rather than creating it twice for both tests.
>
> Done.
>
>> Still skeptical about the use of a PRNG in the tests, see my comments 
>> in a previous email:
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-April/026579.html
>
> Changed to SplittableRandom but collected the intermediate stream into 
> a list which is inspected upon failure to find the value which exposed 
> the misbehavior.