[9] RFR of 8032027: Add BigInteger square root methods

[joe darcy]

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