RFR: 8222460: -XX:+PrintFlagsRanges prints incorrect range

[gerard ziemski]

On 4/16/19 9:45 AM, Per Liden wrote:
>
> On 04/16/2019 04:26 PM, gerard ziemski wrote:
>>
>>
>> On 4/15/19 2:40 PM, Per Liden wrote:
>>>>
>>>> I was wrong in my initial reply.
>>>>
>>>> There are flags with both range and constraint - the entire point 
>>>> of printing the range to the user is to help define valid values, 
>>>> and as you say "We can't expect a user to know what the range of a 
>>>> specific type is on every platform", which applies to both cases.
>>>>
>>>> If the test has an issue with the flag, then the test itself needs 
>>>> to be fixed, by excluding the troublesome flag.
>>>>
>>>> I believe that we should print out the default range in both cases 
>>>> (done in a followup) and we should modify the test to exclude hard 
>>>> to test (troublesome) flags as needed.
>>>
>>> But for flags with constrains functions we have no way of knowing 
>>> the valid range, right? So, the question becomes, is it better for a 
>>> user to get no information (we an empty range) instead of the wrong 
>>> information (we print the default range)? I tend to lean toward 
>>> printing no information. A third alternative would be printing the 
>>> default range for the type, but also add something in the printout 
>>> to indicate that there might be other constraints too.
>>
>> I was thinking about doing something along this line.
>>
>>
>>>
>>>>
>>>> Can't we just exclude "SoftMaxHeapSize" from the test here?
>>>
>>> Sure, I've updated that patch for JDK-8222145 "Add 
>>> -XX:SoftMaxHeapSize flag" to exclude it from testing. Webrev here:
>>>
>>> http://cr.openjdk.java.net/~pliden/8222145/webrev.2
>>>
>>> Looks ok?
>>
>> Looks ok, but why make "allWriteableOptions" static field just to access 
>
> Ok, thanks.
>
>> it in "excludeTestRange()"? Couldn't we simply pass 
>> "allWriteableOptions" as an argument to excludeTestRange()"?
>
> In principle I agree with you, but I'm just following the style in 
> which these tests are written, see 
> test/hotspot/jtreg/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java. 
> If we want to use a different style for these tests we should fix all 
> of them, and I didn't want to do that in this patch. Perhaps something 
> for your followup?

Sounds good.


cheers