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

[Per Liden]

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?

cheers,
Per

> 
>    43    private static void excludeTestRange(List<JVMOption> 
> allWriteableOptions, String optionName) {
>    44         for (JVMOption option: allWriteableOptions) {
>    45             if (option.getName().equals(optionName)) {
>    46                 option.excludeTestMinRange();
>    47                 option.excludeTestMaxRange();
>    48                 break;
>    49             }
>    50         }
>    51     }
> 
> I don't need to see another webrev for that change.
> 
> 
>> I'll withdraw the review for JDK-8222460, and re-assign the bug to 
>> you, and you can update it as you see fit, ok?
> 
> I filed JDK-8222531 as a followup.
> 
> 
> cheers
> 
>