Paul / Florian, This is a combined response. I am making the corrections pointed out by Florian. Unless I hear otherwise, I am going to assume that Paul’s approval is still valid with these corrections included and will push the path on Wednesday. Please see the updated patch:
http://cr.openjdk.java.net/~bpb/8026236/webrev.04/ Thanks, Brian On May 6, 2014, at 1:25 AM, Paul Sandoz <paul.san...@oracle.com> wrote: > On May 5, 2014, at 11:17 PM, Brian Burkhalter <brian.burkhal...@oracle.com> > 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/ >> >> I believe that it addresses all the points of concern aside from testing >> Mersenne primes. >> > > +1 > > Paul On May 6, 2014, at 1:59 AM, Florian Weimer <fwei...@redhat.com> wrote: > On 05/05/2014 11: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/ > > The command line parsing is off by one, I think, it should check for >= > instead of >: > > 54 // Prepare arguments > 55 int upperBound = args.length > 1 ? Integer.valueOf(args[0]) : > DEFAULT_UPPER_BOUND; > 56 int certainty = args.length > 2 ? Integer.valueOf(args[1]) : > DEFAULT_CERTAINTY; > 57 boolean parallel = args.length > 3 ? Boolean.valueOf(args[2]) : > true; I think this happened when I replaced the primes file input with programmatic generation of primes. > I think this line should go, it doesn't match the shifted indexing: > > 98 //bs.set(0, 2, true); I forgot to delete it when I changed to shifted indexing. > There are some very long lines: > > 120 NavigableSet<BigInteger> primes = bs.stream().mapToObj(p -> > BigInteger.valueOf(p + 2)).collect(toCollection(TreeSet::new)); > > 180 List<BigInteger> bigInts = (new > SplittableRandom()).ints(NUM_NON_PRIMES, 2, > maxPrime).mapToObj(BigInteger::valueOf).filter(b -> > !b.isProbablePrime(certainty)).collect(toList()); Lines split into several at worst not much longer than 80 characters. > The arithmetic in checkPrime() is unnecessary because isProbablePrime() will > never report a prime as a non-prime (the Monte-Carlo vs Las Vegas thing), so > the count will always be exact because you feed it only primes. Good point: check removed. > It is impossible to check for the expected number of failures (non-primes > reported as primes) in checkNonPrimes() because this would result in a test > that fails sporadically. There is no change as a result of this comment.
