Hi Brian,
On 11/25/2015 12:02 PM, Brian Burkhalter wrote:
On Nov 16, 2015, at 10:00 AM, joe darcy <joe.da...@oracle.com> wrote:
Returning to this review…
Likewise …
Please refer to the updated patch at
http://cr.openjdk.java.net/~bpb/8032027/webrev.02/.
A few comments and suggestions:
2452 public BigInteger[] sqrtAndRemainder() {
2453 BigInteger s = sqrt();
2454 BigInteger r = this.subtract(s.square());
2455 return new BigInteger[] {s, r};
2456 }
It that "remainder >= 0" would be a good assert to add before line 2455.
Assert added.
As a stylistic matter, in MutableBigInteger I would prefer a direct check of
bitLength rather than relying on the exception being thrown from
longValueExact. Something like
if (bitLength < 63)
// use double initial approx
else
// use other technique
Changed as suggested.
Revised code looks fine.
In the tests, use of lambda could allow the core testing logic to be shared.
I don’t know precisely what was expected but the test has been modified to
reduce redundant code.
Another comments on the tests. For code like
347 // square root of n^2 -> n
348 BigInteger[] actual = n2.sqrtAndRemainder();
349 if (actual[0].compareTo(n) != 0) {
350 failCount++;
351 printErr("sqrtAndRemainder()[0]", n, actual[0]);
352 }
353 if (actual[1].compareTo(BigInteger.ZERO) != 0) {
354 failCount++;
355 printErr("sqrtAndRemainder()[1]", BigInteger.ZERO,
actual[1]);
356 }
357
358 // square root of n^2 + 1 -> n
359 BigInteger n2up = n2.add(BigInteger.ONE);
360 actual = n2up.sqrtAndRemainder();
361 if (actual[0].compareTo(n) != 0) {
362 failCount++;
363 printErr("sqrtAndRemainder()[0]", n, actual[0]);
364 }
365 if (actual[1].compareTo(BigInteger.ONE) != 0) {
366 failCount++;
367 printErr("sqrtAndRemainder()[1]", BigInteger.ONE,
actual[1]);
368 }
The "if (...) " logic that is repeated a few times in this method could
be pulled out into its own method, possibly one structured a bit
differently to return the number of errors.
I think it would be acceptable to push the tests in their current state,
but I would prefer to see a little more refactoring.
I would have expected some tests to directly work off of the definition
of integer square root, namely that k^2 is less or or equal to the
argument but (k+1)^2 is larger.
Thanks,
-Joe
