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

[gerard ziemski]

Thank you for the review.


On 08/04/2015 12:35 PM, Coleen Phillimore wrote:
>
> 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.

Done.


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

Done as well, I changed the APIs to a consistent form of "Api(..., 
value, verbose)".

I will post webrev3 after testingis done.


cheers