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

[joe darcy]

Hi Brian,

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.)

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...").

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.

Thanks,

-Joe


On 12/30/2014 8:33 AM, Brian Burkhalter wrote:
> I’ve added the suggested @throws tags here:
>
> http://cr.openjdk.java.net/~bpb/4026465/webrev.01/
>
> Thanks,
>
> Brian
>
> On Dec 30, 2014, at 2:33 AM, Stephen Colebourne <scolebourne at joda.org> wrote:
>
>> Just to note that an IndexOutOfBoundsException is mentioned in the
>> text (line 274, 350) but there is no matching @throws clause. The
>> IndexOutOfBoundsException could have a message added.
>>
>> In general, I would expect an offset/length overload for most methods
>> that take an array, so this seems like a sensible request.