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