JDK 9 RFR of 4026465: Provide more byte array constructors for BigInteger

[Joseph D. Darcy]

Hi Brian,

The new version looks fine. One suggestion to consider: creating a small 
private helper method to do the len, off, array-length check. (The two 
copies of the logic are slightly different.)

Thanks,

-Joe

On 1/2/2015 4:09 PM, Brian Burkhalter wrote:
> Hello all,
>
> Thanks for the comments. A new patch is here:
>
> http://cr.openjdk.java.net/~bpb/4026465/webrev.02/
>
> On Dec 30, 2014, at 6:15 PM, joe darcy <joe.darcy at oracle.com> wrote:
>
>> The new changes generally look good. A few comments, for the new code like
>>
>> 291         } else if ((off < 0) || (off > val.length) || (len < 0) ||
>> 292                    ((off + len) > val.length) || ((off + len) < 0)) {
>> 293             throw new IndexOutOfBoundsException();
>>
>> it is not immediately clear that the arithmetic on line 292 won't have some inappropriate overflow behavior. A comment stating why "off + len" will behave appropriately (assuming it does behave appropriately ;-) would help. (By line 292, both off and len are non-negative so that should limit the case analysis.)
> Logic updated.
>
>> It might be worthwhile for all the BigInteger constructors which take array arguments to state something about the thread-safely behavior ("arrays are assumed to be unchanged for the duration of the constructor call...”).
> Verbiage added.
>
>> Do have have any code coverage information for the new code by the regression test? It would be good to know whether or not all the guard conditions are properly being executed.
> No coverage information, but I added some tests for the guard conditions and slightly changed the correct-value part of the test.
>
> On Dec 30, 2014, at 6:42 PM, Paul Benedict <pbenedict at apache.org> wrote:
>
>> Please add @since 1.9 to the new constructors.
> Done.
>
> On Jan 2, 2015, at 1:57 AM, Alan Bateman <Alan.Bateman at oracle.com> wrote:
>
>> I assume this can be reduced down to:
>>   if (off < 0 || len < 0 || (off > val.length - len)) { ... }
>
> But then if len > val.length, the third inequality tests ‘off’ for being greater than a negative value. Please see the updated patch.
>
> Thanks,
>
> Brian