RFR (M): 8112746: Followup to JDK-8059557 (JEP 245: Validate JVM Command-Line Flag Arguments)

[Coleen Phillimore]

I've reviewed this code.  My only two comments are from two replies in 
this thread:

>> ------------------------------------------------------------------------------
>> src/share/vm/runtime/commandLineFlagConstraintsCompiler.cpp
>> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>> src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp
>> src/share/vm/runtime/commandLineFlagRangeList.cpp
>>
>> Duplicated print_error_if_needed.
>
> Right, I wasn't sure whether to factor this method out or not. I 
> decided to have it local because it's trivial, but I will factor it 
> out into src/share/vm/runtime/commandLineFlagRangeList.hpp/.cpp though 
> that will require all the constraint files to include the 
> commandLineFlagRange.List.hpp
>

Yes, please factor out the error message functions.

> ------------------------------------------------------------------------------
>> - Passing values by value, not pointer, to constraint functions. It
>> was thought originally that me way want to “fix” the values, but it
>> was decided to be out of scope for the JEP. (requested by Coleen)
> I keep waffling about this, so I'll go ahead an mention it, even
> though I don't yet have a strong opinion either way.
>
> For the writer of a constraint function, not needing to deal with a
> pointer is clearly better.
>
> However, I actually kind of thought it was a feature that the values
> were passed by pointer reference rather than by value (though the
> pointer should be const-qualified if no "fixup" capability is
> intended).  That prevents certain kinds of bugs due to implicit
> conversions; the types involved here provide pretty much all
> conversions between pairs of types.  I'm also concerned about bugs
> resulting from reversal of the value and verbose arguments, especially
> since their order is not consistent throughout the internal APIs.
>
> There are two different pathways to these constraint functions, one of
> which is still using (and needs to use) pointers.  This change
> dereferences these pointers, rather than taking the address of values
> on the other path.  So which approach is used is a wash from the
> caller's perspective.

I don't see the path where we need to pass pointers and where this 
change dereferences these pointers.  I think it's an improvement not to 
have these pointers and dereferenced when used. I like this change.

I agree with the concern about bugs reversing the arguments, 
particularly since the constraint function has arguments in the opposite 
order:

-  Flag::Error apply_size_t(size_t* value, bool verbose) {
+  Flag::Error apply_size_t(size_t value, bool verbose) {
      return _constraint(verbose, value);
    }
  };

Would it be difficult to switch the order of arguments with these apply* functions so they match?

Otherwise, unfortunately in C++ we have lots of places where we have to worry about implicit conversions with parameter passing in the code, and I would prefer using enums to defeat parameter type ambiguity than passing as a pointer where one isn't needed.   But I don't think the risk of bugs with this code at this moment needs to do something about it.  If there were 5 'int' convertible parameters (or 4 bools), I would agree more strongly with you.

Thanks,
Coleen