Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments

[Gerard Ziemski]

Thank you David for more feedback. My answers are provided inline:
> Subject:	Re: Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
> Date:	Fri, 29 May 2015 14:39:11 +1000
> From:	David Holmes<david.holmes at oracle.com>
> Organization:	Oracle Corporation
> To:	Gerard Ziemski<gerard.ziemski at oracle.com>,hotspot-dev at openjdk.java.net  Developers<hotspot-dev at openjdk.java.net>
> CC:	Coleen Phillimore<coleen.phillimore at oracle.com>, Dmitry Dmitriev<dmitry.dmitriev at oracle.com>, Kim Barrett<kim.barrett at oracle.com>, Alexander Harlap<alexander.harlap at oracle.com>
>
> Hi Gerard,
>
> Meta-comment: I had expected to see constraint functions subsume range
> checks - ie that the range check would be folded into the constraints
> function so that all the constraints are clearly seen in one place. Not
> sure splitting things across two checks aids in understandability. This
> isn't a blocker but I'd be interested to hear other views on this.
>
Keeping ranges separate from constraints allows us to easily print them out on demand - a requirement from SQA point of view (see PrintFlagsRanges)


> A general comment, note that in cases like:
>
>   "intx %s="INTX_FORMAT" is outside the allowed range [ "INTX_FORMAT"
> ... "INTX_FORMAT" ]\n",
>
> we now need to add spaces between the macros like INTX_FRORMAT and the
> double-quotes - see 8081202. Not sure who will be pushing first but it's
> an easy fix to make.

Done.


> A few minor specific comments:
>
> src/share/vm/runtime/commandLineFlagConstraintsCompiler.hpp
>
> Has the comment:
>
> 32  * Here we have runtime arguments constraints functions,
>
> - should say 'compiler'
>
Done.
  

> ---
>
> src/share/vm/runtime/commandLineFlagConstraintsCompiler.cpp
>
> Not obvious all the includes are needed ie java.hpp and os.hpp


Done.

>
> ---
>
> src/share/vm/runtime/commandLineFlagConstraintsGC.hpp
>
> Has the comment:
>
>   32  * Here we have runtime arguments constraints functions,
>
> - should say 'GC'
>
> I'm a little surprised the #if INCLUDE_ALL_GCS only covers the G1
> options. I guess we don't as aggressively exclude the code for the other
> GC's.

Done.

> ---
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>
> Not obvious all the includes are needed ie java.hpp and os.hpp, and
> c1/c2 globals?

Done.

> ---
>
> Nothing else jumped out at me, but I didn't verify the non-runtime
> constraints.

Thank you - I will be posting up a new webrev soon.



cheers