RFR: 8081833: Clean up JVMFlag getter/setter code

[Gerard Ziemski]

On Tue, 15 Sep 2020 05:01:13 GMT, Ioi Lam <iklam 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.

Changes requested by gziemski (Committer).

src/hotspot/share/runtime/flags/jvmFlagLimit.cpp line 156:

> 154:   for (int i = 0; i < NUM_JVMFlagsEnum; i++) {
> 155:     JVMFlagsEnum flag_enum = static_cast<JVMFlagsEnum>(i);
> 156:     if (get_range_at(flag_enum) != NULL &&

VMFlagAccess::check_range(JVMFlag::flag_from_enum(flag_enum) will check for "get_range_at(flag_enum) != NULL", so no
need for this check here?

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?

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?

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. ?

src/hotspot/share/runtime/flags/jvmFlag.hpp line 128:

> 126:     TYPE_ccstrlist,
> 127:     NUM_FLAG_TYPES
> 128:   };

Are those macros really needed here and better than simply saying:

  enum FlagType : int {
    TYPE_bool,
    TYPE_int,
    TYPE_uint,
    TYPE_intx,
    TYPE_uintx,
    TYPE_uint64_t,
    TYPE_size_t,
    TYPE_double,
    TYPE_ccstr,
    TYPE_ccstrlist,
    NUM_FLAG_TYPES
  };

src/hotspot/share/runtime/flags/jvmFlag.hpp line 259:

> 257:   bool is_writeable() const       { return is_manageable();                      }
> 258:   // All flags except "manageable" are assumed to be internal flags.
> 259:   bool is_external() const        { return is_manageable();                      }

Do we need is_writeable() and/or is_external() if all they do is return is_manageable() ?

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

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