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

[gerard ziemski]

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 
it in "excludeTestRange()"? Couldn't we simply pass 
"allWriteableOptions" as an argument to excludeTestRange()"?

   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