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

[David Holmes]

Hi Ioi,

On 27/10/2020 8:32 am, Ioi Lam wrote:
> On Fri, 23 Oct 2020 06:33:06 GMT, Ioi Lam <iklam at openjdk.org> wrote:
>> _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.

Yes separate RFE. This might be necessary/desirable but at a minimum it 
should be clearly documented when ergonomics can override an explicit 
user setting.

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

That is a pity. I'm not sure I agree with the overall approach of making 
enums all top-level just to minimise the number of includes needed. Code 
structure is more important to me than shaving a few seconds off build 
times.

>>> 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)

Okay.

Thanks,
David

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