RFR: 8224878: Use JVMFlag parameters instead of name strings

[David Holmes]

Hi Stefan,

Just in relation to the find_flag(name) versus find_flag(name, len) 
issue, I'm trying to see the simplest thing to "flip" so that it makes 
more sense, but to be honest I don't understand why we use the 
find_flag(name, len) form in the places that we do. For example:

JVMFlag::Error JVMFlag::boolAtPut(const char* name, size_t len, bool* 
value, JVMFlag::Flags origin) {
   JVMFlag* result = JVMFlag::find_flag(name, len);
   return boolAtPut(result, value, origin);
}

won't find a locked flag - but why not? I don't see why this should be a 
function that only applies to unlocked flags. ??

But given we are talking about locked versus unlocked flags then 
find_flag_unlocked() would seem more appropriate than 
find_flag_unconstrained().

More tomorrow ...

Cheers,
David

On 28/05/2019 10:44 pm, Stefan Karlsson wrote:
> And here's the webrev:
>   http://cr.openjdk.java.net/~stefank/8224878/webrev.01/
> 
> Thanks,
> StefanK
> 
> On 2019-05-28 13:35, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to use JVMFlag parameters instead of name 
>> strings in the JVM flag handling code.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8224878
>>
>> The intention is to reduce the places where the API uses "const char* 
>> name" parameters to describe a JVM flag, and instead use a JVMFlag* 
>> when performing various flag handling code. The places where we 
>> translate from "const char* name" to JVMFlag* has been pushed to the 
>> outer layers of the code, which explicitly uses the JVMFlag::find_flag 
>> functions.
>>
>> This allows us to replace strcmp with simple JVMFlag pointer equality 
>> checks in jvmFlagRangeList.cpp and jvmFlagConstraintList.cpp.
>>
>> This gets rid of the need to store a typed pointer to the flag in 
>> jvmFlagRangeList.cpp and jvmFlagConstraintList.cpp.
>>
>> This gets rid of the the need to propagate the 'allow_locked' and 
>> 'return_flag' parameters down the call chain. For example, see the 
>> changes to JVMFlag::<type>At and JVMFlag::<type>AtPut:
>> -  static JVMFlag::Error intAt(const char* name, int* value, bool 
>> allow_locked = false, bool return_flag = false)      { return 
>> intAt(name, strlen(name), value, allow_locked, return_flag); }
>> +  static JVMFlag::Error intAt(const JVMFlag* flag, int* value);
>>    static JVMFlag::Error intAtPut(JVMFlag* flag, int* value, 
>> JVMFlag::Flags origin);
>> -  static JVMFlag::Error intAtPut(const char* name, size_t len, int* 
>> value, JVMFlag::Flags origin);
>> -  static JVMFlag::Error intAtPut(const char* name, int* value, 
>> JVMFlag::Flags origin)   { return intAtPut(name, strlen(name), value, 
>> origin); }
>>
>>   It changes the JVMFlag::find_flag API. The way it uses default 
>> values have a surprising effect for users of those functions. 
>> find_flag(name) searches among all flags (even locked and constants), 
>> while find_flag(name, strlen(name)) doesn't return locked or constants 
>> flags. To make it less likely to accidentally call the wrong version, 
>> this has been changed to the following:
>> * JVMFlag::find_flag(name) - Fetches the flag if it is available 
>> (unlocked and not constant).
>> * JVMFlag::find_flag_unrestricted(name) - Fetches the flag even if it 
>> is locked or a constant.
>>
>> Removed the unused JVMFlag::wasSetOnCmdline.
>>
>> Small cleanups
>> - Renamed address_of_flag to flag_from_enum
>> - Renamed local JVMFlag* variable from results to flag
>> - Use initializer lists
>> - Removed superfluous semicolons
>>
>> I don't mind splitting patch up into more than one RFE, if that seems 
>> more appropriate.
>>
>> Tested with:
>> - test/hotspot/jtreg/runtime/CommandLine
>> - tier1 (tier2-3 80% done)
>>
>> Suggestions on other testing is appreciated.
>>
>> Thanks,
>> StefanK