RFR: 8253640: Make MEMFLAGS an enum class

[Thomas Stuefe]

On Mon, 28 Sep 2020 11:49:37 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> I'd like to propose that we make MEMFLAGS an enum class, to prevent them from being implicitly converted to integers.
> 
> I've been refactoring code that uses MEMFLAGS and have found that the functions often take integers and/or use default
> values. Because of this the compiler won't always tell you if you mess up the order. If we make the MEMFLAGS more type
> safe, the compiler will complain immediately if you send in the arguments in the wrong order.  Changes in the patch:
> 
> 1) G1 is the only code that uses MemoryType instead of the MEMFLAGS typedef. I changed those to make the code base
> uniform w.r.t. this.  2) Added some code generation so that we still can use mtGC, and don't have to write
> MEMFLAGS::mtGC. 3) Some stricter checks around the values stored in MEMFLAGS variables. There was a masking of 0xFF
> that I removed in favor of an assert. Need to run some more extensive testing see if there was a need for this masking.
> 4) Removed unused by_index
> Notes:
> 1) I didn't specify the underlying type of the enum, but it's clear for its usage that code will start to break if the
> value range is larger than 8 bits. Maybe something to clean up in a separate patch? 2) The code sometimes talks about
> "flags" and sometimes about "memory type", it's quite clear that the name changed, but the comments and names didn't
> get cleaned up accordingly.

Hi Stefan,

I was just looking this morning at this enum and wished it were a class. This will make reworking the reservation APIs
less error prone.

Found some small nits, but leave it up to you if you change anything.

(For some reason I always believed that using enum class would prevent us from using sentinel values but for the life
of me I cannot remember why I thought this...)

Thanks, Thomas

src/hotspot/share/services/nmtCommon.hpp line 55:

> 53:   }
> 54:
> 55:   // Check if flag value is a valid MEMFLAGS enum value

Small nit, can you please clarify that this includes "mtNone"?

src/hotspot/share/memory/allocation.hpp line 154:

> 152:  * Memory types
> 153:  */
> 154: enum class MEMFLAGS {

Remark (possible for a later change): As you wrote, I think using uint8 would be okay and safer here.

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

Marked as reviewed by stuefe (Reviewer).

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