[9] RFR of 8032027: Add BigInteger square root methods
joe darcy
joe.darcy at oracle.com
Sun Nov 29 18:01:22 UTC 2015
Hi Brian,
On 11/25/2015 12:02 PM, Brian Burkhalter wrote:
> On Nov 16, 2015, at 10:00 AM, joe darcy <joe.darcy at 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
More information about the core-libs-dev
mailing list