Thanks Roger,
Updated.
http://cr.openjdk.java.net/~nishjain/8210583/webrev.05/
Regards,
Nishit Jain
On 18-01-2019 21:13, Roger Riggs wrote:
Hi Nishit,
Looks good, with a minor fix.
ok, the rationale for MAX_VALUE-2 make sense.
TestEncodingDecodingLength: Line 61 and 68,
The error message will be more readable with a ": " before the
methodname is appended.
Thanks, Roger
On 01/18/2019 06:03 AM, Nishit Jain wrote:
Hi Roger, Naoto,
> The size of Integer.MAX_VALUE - 2 is implementation specific,
I think more typically max -8 is used to get the biggest allocation.
When Integer.MAX_VALUE - 8 is used, the decode methods do not throw
the added OOME, because the computed value does not overflow
Integer.MAX_VALUE, so to test the added OOME condition need to use
Integer.MAX_VALUE - 2.
Updated the webrev with rest of the suggestions
http://cr.openjdk.java.net/~nishjain/8210583/webrev.04/
Regards,
Nishit Jain
On 18-01-2019 02:44, Naoto Sato wrote:
I agree with 'withOutputParam' comment. It does seem to require some
explanation. Same for the newly introduced return value -1.
The test:
46 // A separate output array is not required, as it is
not used and
47 // the exception is thrown beforehand while calculating
the size
48 // of the encoded bytes using input array
This seems to make an assumption on the implementation. Test should
not depend on the internal impl.
Naoto
On 1/17/19 8:14 AM, Roger Riggs wrote:
Hi Nishit,
In the test, there are a couple of RuntimeExceptions with messages
that start with "As expected"...
That's counter intuitive, that a failure of the test is reported
with a message saying, its normal!
I would use a message like: "XXException should have been thrown..."
The size of Integer.MAX_VALUE - 2 is implementation specific,
I think more typically max -8 is used to get the biggest allocation.
java/util/Base64.java:
Lines 335-342: This optimization is not mentioned in the bug
report and
should be a separate review.
245, 686: in outLength(), the 2nd parameter would be easier to
understand
as 'throwOOME', meaning to throw OOM if length is too large.
The 'withOutputParam' only has any meaning in the context of the
caller.
And even for the private method write the javadoc describing the
behavior.
It also makes the call sites clearer, the argument will be true to
throw.
Thanks, Roger
On 01/17/2019 02:07 AM, Nishit Jain wrote:
Hi,
Please review the fix for 8210583
Bug: https://bugs.openjdk.java.net/browse/JDK-8210583
Webrev: http://cr.openjdk.java.net/~nishjain/8210583/webrev.03/
CSR: https://bugs.openjdk.java.net/browse/JDK-8215633
Issue: Base64.Encoder.encode and Base64.Decoder.decode methods
incorrectly throw unspecified exception e.g.
NegativeArraySizeException, when the input byte array size is too
large such that the calculated output byte size goes beyond the
max-integer boundary and wraps around.
Fix: Throw an OutOfMemoryError if the output byte array/buffer or
memory can not be allocated. There is an unrelated change in
encodeToString(byte[]) where a string instance is created using
JavaLangAccess.newStringNoRepl(byte[], ISO_8859_1)instead of
string constructor, to save memory.
Regards,
Nishit Jain
