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.da...@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 <pbened...@apache.org> wrote: > Please add @since 1.9 to the new constructors. Done. On Jan 2, 2015, at 1:57 AM, Alan Bateman <alan.bate...@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
