[15] RFR: 8239520: ValueRange.of(long, long, long) does not throw IAE on invalid inputs

[naoto.sato at oracle.com]

Hi Roger,

Thank you for the review. Please find my comments below.

On 2/21/20 11:42 AM, Roger Riggs wrote:
> In the implementation ValueRange.of: 148-150; I would omit the check 
> here since
> it invokes the 4 arg form and does the same check.
> The difference in exception message is not worth the extra code.
> Though it would help if it said that the 3 arg form is the same as the 4 
> arg form with the same first two args.

I followed the pattern in the 2 arg form which adds a condition with its 
own exception message, and also invokes 4 arg form.

> 
> In the test TestDateTimeValueRange.java, the separate test case is 
> (mostly) redundant with the added case at line 187.
> (Though that assumes the 3 arg form calls the 4 arg form; as does the 
> lack of any other testing of the 3 arg form).

Since this is a test, I would rather not rely on the implementation 
detail that 3 arg form calls into 4 arg form. Eventual redundancy in 
test would not be an issue, IMO.

> 
> Also,
> It might be useful to verify that each of the test cases 179-186 trigger 
> only 1 of the range checks.
> If a test case would fail more than 1 of the conditions then it might be 
> never be testing each if statements.

I specifically added variations that only verify each condition. 
Modified the test case:

http://cr.openjdk.java.net/~naoto/8239520/webrev.01/

Naoto

> 
> Thanks, Roger
> 
> 
> On 2/20/20 5:28 PM, naoto.sato at oracle.com wrote:
>> Hi,
>>
>> Please review the fix to the following issue:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8239520
>>
>> The proposed changeset is located at:
>>
>> https://cr.openjdk.java.net/~naoto/8239520/webrev.00/
>>
>> This change lets 3-arg ValueRange.of() method thrown an IAE on invalid 
>> inputs, as well as corrects the method description of 4-arg 
>> ValueRange.of() method which lacks an IAE condition. I also filed a 
>> CSR for this change accordingly:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8239574
>>
>> Naoto
>