RFR: 8081833: Clean up JVMFlag getter/setter code

[Ioi Lam]

On Tue, 15 Sep 2020 19:40:58 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> This is a follow to [JDK-8243208 Clean up JVMFlag implementation](https://bugs.openjdk.java.net/browse/JDK-8243208).
>> 
>> I try to use templates and inheritance (and a bit of macros) to get rid of the large amount of duplicated code in the
>> JVMFlag getters/setters.
>> Stefan Karlsson and I have tried this several times already, but I think the current attempt probably has the least
>> amount of hacks and most functionality:
>> - Type safety is enforced up to the point of loading/storing `JVMFlag::_addr`, without any unchecked typecasts.
>> - Runtime type checking is implemented by the `JVM_FLAG_TYPE(t)` macro (see jvmFlagAccess.hpp), by passing along an
>>   integer (`JVMFlag::TYPE_int`, etc). This is more efficient than previous attempts that passed along a function pointer
>>   (see http://cr.openjdk.java.net/~stefank/8081833/webrev.04/)
>> - The old `JVMFlag::xxxAtPut` functions were undocumented. There are actually two groups that have different usage. I
>>   added documentation in jvmFlagAccess.hpp to explain this.
>> - I got rid of the `JVMFlagEx` class and moved the code into `JVMFlag`. This is possible with C++11 which allows forward
>>   declaration of `enum JVMFlagsEnum : int;`
>> - I changed `JVMFlag::_type` from a string into an enum. I am surprised this had lasted for over 20 years.
>> - I also changed `JVMFlag` from a struct into a class and made some fields private. I probably will do more cleanup in a
>>   separate RFE.
>> 
>> Please start with jvmFlagAccess.hpp, jvmFlagAccess.hpp and then globals_extension.hpp.
>> 
>> Probably not everyone is in love with the `JVM_FLAG_TYPE(t)` macro. I'll be happy to try out alternatives.
>
> src/hotspot/share/runtime/globals_extension.hpp line 38:
> 
>> 36: #define FLAG_MEMBER_ENUM_(name) FLAG_MEMBER_ENUM(name),
>> 37:
>> 38: #define FLAG_MEMBER_ENUM_DECLARE(type, name, ...)  FLAG_MEMBER_ENUM_(name)
> 
> We have other macros named:
> 
> DECLARE_PRODUCT_FLAG
> MATERIALIZE_PRODUCT_FLAG
> 
> Can we rename this one as ENUMERATE_FLAG_MEMBER, (with the verb first in the name) to be more clear and consistent?

How about DEFINE_FLAG_MEMBER_ENUM? In C++ terminology (https://en.cppreference.com/w/cpp/language/enum), we are
building an "comma-separated list of enumerator definitions".

> src/hotspot/share/runtime/globals_extension.hpp line 59:
> 
>> 57:   }
>> 58:
>> 59: #define FLAG_MEMBER_SET_DECLARE(type, name, ...) FLAG_MEMBER_SET_(type, name)
> 
> We have other macros named:
> 
> DECLARE_PRODUCT_FLAG
> MATERIALIZE_PRODUCT_FLAG
> 
> Can we rename this one as SET_FLAG_MEMBER or DECLARE_FLAG_MEMBER, (with the verb first in the name) to be more clear
> and consistent?

Here, we are defining a "flag member setter function". I think we should rename to
#define FLAG_MEMBER_SETTER(name) Flag_##name##_set
#define FLAG_MEMBER_SETTER_(type, name) \
  inline JVMFlag::Error FLAG_MEMBER_SETER(name)(type value, JVMFlag::Flags origin) { \
    return JVMFlagAccess::set<JVM_FLAG_TYPE(type)>(FLAG_MEMBER_ENUM(name), value, origin); \
  }

#define DEFINE_FLAG_MEMBER_SETTER(type, name, ...) FLAG_MEMBER_SETTER_(type, name)

ALL_FLAGS(DEFINE_FLAG_MEMBER_SETTER, ...

> src/hotspot/share/runtime/flags/jvmFlag.cpp line 523:
> 
>> 521: #define PRODUCT_FLAG_INIT(   type, name, value, ...) JVMFlag(FLAG_MEMBER_ENUM(name), FLAG_TYPE(type), XSTR(name),
>> (void*)&name, PRODUCT_KIND,    __VA_ARGS__), 522: #define PRODUCT_FLAG_INIT_PD(type, name,        ...)
>> JVMFlag(FLAG_MEMBER_ENUM(name), FLAG_TYPE(type), XSTR(name), (void*)&name, PRODUCT_KIND_PD, __VA_ARGS__), 523: #define
>> NOTPROD_FLAG_INIT(   type, name, value, ...) JVMFlag(FLAG_MEMBER_ENUM(name), FLAG_TYPE(type), XSTR(name), (void*)&name,
>> NOTPROD_KIND,    __VA_ARGS__),
> 
> We have other macros named:
> 
> DECLARE_PRODUCT_FLAG
> MATERIALIZE_PRODUCT_FLAG
> 
> Can we rename these macros:
> 
> DEVELOP_FLAG_INIT --> INITIALIZE_DEVELOP_FLAG
> DEVELOP_FLAG_INIT_PD --> INITIALIZE_PD_DEVELOP_FLAG
> etc. ?

OK

-------------

PR: https://git.openjdk.java.net/jdk/pull/163