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/ 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.da...@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.san...@oracle.com> wrote: > On May 3, 2014, at 6:55 PM, Joe Darcy <joe.da...@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.
