RFR: 8239520: ValueRange.of(long, long, long) does not throw IAE on invalid inputs
Roger.Riggs at oracle.com
Mon Feb 24 14:17:17 UTC 2020
The updates look fine.
On 2/21/20 6:03 PM, naoto.sato at oracle.com wrote:
> 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.
>> 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:
>> Thanks, Roger
>> On 2/20/20 5:28 PM, naoto.sato at oracle.com wrote:
>>> Please review the fix to the following issue:
>>> The proposed changeset is located at:
>>> 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:
More information about the core-libs-dev