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

[Kim Barrett]

On Aug 4, 2015, at 1:35 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
>>> - 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.

See the apply_constraint_and_check_range_XXX (static) functions in globals.cpp.

Hm, these functions don’t modify the pointer value either, so could either be called with a non-pointer.  And I see another thing there that I don’t understand.  I guess I have some more comments to write up.

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

Yes, that’s what I was worried about.  Gerard says he’s fixed that inconsistency though.

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

I did say I was waffling about this.  With the argument order inconsistency going away, and having looked again at the places where pointers are still used (with further suggestions there forthcoming), I think the benefit for writers of constraint functions pretty strongly outweighs my concerns, and I’m ok with this change.