RFR: 8255285: Move JVMFlag origins into a new enum JVMFlagOrigin

[Ioi Lam]

On Fri, 23 Oct 2020 06:33:06 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> Many JVM function take an `JVMFlag::Flags` parameter to indicate the origin of the flag -- i.e., "who is setting this flag". E.g., in arguments.hpp:
> 
> static bool parse_argument(const char* arg, JVMFlag::Flags origin);
> 
> However, `JVMFlag::Flags` contains many other bits that are unrelated to the origin. We should add a new enum `JVMFlagOrigin` that has only the valid values for the origin. This makes it possible to do more type-safety checks at C++ compilation time.
> 
> This patch also renamed the confusing bit `JVMFlag::ORIG_COMMAND_LINE` to `WAS_SET_IN_COMMAND_LINE` and added documentation, so that it won't be confused with `JVMFlagOrigin::COMMAND_LINE`.

> _Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-dev](mailto:hotspot-dev at openjdk.java.net):_
> 
> Hi Ioi,
> 
> On 23/10/2020 4:52 pm, Ioi Lam wrote:
> > This patch also renamed the confusing bit `JVMFlag::ORIG_COMMAND_LINE` to `WAS_SET_IN_COMMAND_LINE` and added documentation, so that it won't be confused with `JVMFlagOrigin::COMMAND_LINE`.
> 
> I'm still confused :) Why are we reporting "command line" for a flag
> that was ergonomically set, or vice-versa? Surely a flag is either set
> via the command-line or via ergonomics but not both ??

We have code like this:

void JVMFlag::print_origin(outputStream* st, unsigned int width) const {
    case JVMFlagOrigin::ERGONOMIC:
      if (_flags & WAS_SET_IN_COMMAND_LINE) {
        st->print("command line, ");
      }
      st->print("ergonomic"); break;

So if FLAG_SET_ERGO changes a flag that was specified in the command-line, we will print out "command line, ergonomic". 

> I was under the
> assumption that ergonomics should not touch a flag explicitly set on the
> command-line as that defeats the purpose of setting it.

I have no idea why this is the case. Maybe ergonomics is allowed to "fine tune" user-specified values? Anyway, if we want to change this, we should do it in a separate RFE.

> > enum class JVMFlagOrigin
> 
> Why not define this as
> 
> enum class Origin
> 
> inside class JVMFlag, so that it is then referred to as JVMFlag::Origin?

The reason is to allow `JVMFlagOrigin` to be used in a forward declaration without including jvmFlag.hpp. See vmEnums.hpp.

A nested enum like `JVMFlag::Origin` cannot be forward-declared.

> > static const JVMFlagOrigin DEFAULT          = JVMFlagOrigin::DEFAULT;
> 
> Why is this needed?? To avoid re-typing JVMFlagOrigin?

Yeah, but I removed this in the latest version [53fed1b](https://github.com/openjdk/jdk/pull/823/commits/53fed1b00784763c873bf81475430e280f06d72c)

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

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