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 <scolebou...@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.
