RFR: JDK-8151604: Rely on options range checking rather than explict checks

[Bengt Rutisson]

Hi Sangheon,

On 2016-03-11 22:06, sangheon wrote:
> Hi Bengt,
>
> On 03/11/2016 04:38 AM, Bengt Rutisson wrote:
>>
>> Hi Derek,
>>
>> Thanks for looking at this!
>>
>> On 2016-03-10 17:03, Derek White wrote:
>>> On 3/10/16 4:59 AM, Bengt Rutisson wrote:
>>>>
>>>> Hi everyone,
>>>>
>>>> Could I have a couple of reviews for this change?
>>>>
>>>> http://cr.openjdk.java.net/~brutisso/8151604/webrev.00/
>>>> https://bugs.openjdk.java.net/browse/JDK-8151604
>>>>
>>>> With the new range checking in the argument parsing we don't need 
>>>> to have explicit checks in the code to handle invalid arguments.
>>>>
>>>> Thanks,
>>>> Bengt
>>>
>>> Great idea. Only comment is that this can go further (I only checked 
>>> g1CollectorPolicy):
>>>
>>> g1CollectorPolicy.cpp:
>>> - Line 186, can change tests of MaxGCPauseMillis to guarantee(), 
>>> because:
>>>           range(1, max_uintx)
>>
>> Fixed.
>>
>>> - Line 192, can change tests of GCPauseIntervalMillis to 
>>> guarantee(), because:
>>>           OK, this doesn't have a range defined, but it should: 
>>> range(1, max_uintx)
>>
>> This code only checks that it is >= 1, which is exactly what 
>> GCPauseIntervalMillisConstraintFunc does. So, I changed this to a 
>> guarantee as well.
>>
>>>
>>> The checks from lines 220-228 are probably covered by the constraint 
>>> functions (MaxGCPauseMillisConstraintFunc(), etc), but this RFE is 
>>> about ranges :-)
>>
>> I changed those as well, even if they are constraint based rather 
>> than ranges.
>>
>>>
>>>  - Line 293, can change tests of SurvivorRatio to guarantee(), because:
>>>             range(1, max_uintx-2)
>>
>> Fixed.
>>
> Looks good to me as well.
> Thanks for fixing this.

Thanks for looking at this!

>
>> There may be more of those that we can fix, but I think this is a 
>> good start. I mostly wanted to get rid of the warning() calls.
> I agree that this is a good start.
>
> FYI, we already have the CR that covers this.
> https://bugs.openjdk.java.net/browse/JDK-8133649

Ah, I didn't know about that one. Good that this is tracked somewhere.

Thanks,
Bengt
>
> Thanks,
> Sangheon
>
>
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~brutisso/8151604/webrev.01/
>>
>> Diff compared to last version:
>> http://cr.openjdk.java.net/~brutisso/8151604/webrev.00-01.diff/
>
>
>>
>> Thanks,
>> Bengt
>>
>>>
>>>  - Derek
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160313/30cabe96/attachment.htm>