RFR: 8066821(S) Enhance command line processing to manage deprecating and obsoleting -XX command line arguments

[Derek White]

Hi All,

I need another review, especially someone from runtime. Coleen, do you 
want crack at it or can you forward as needed?

Another version for review:
http://cr.openjdk.java.net/~drwhite/8066821/webrev.01

Thank you Thomas for your review.

I lost your email somehow but recovered the text. My comments inline below:
> arguments.cpp:
>   - in the big comment please avoid adding special headers/footers to
> sections of the file. Over time they just get orphaned and waste up
> space.
OK
>   - I would prefer to use "special" comments like "/**". We "mostly"
> also only ever use the "//" comment style for explanation in cpp files.
> (Waiting for somebody to mention that in file XY we do differently -
> okay just saw one in arguments.cpp, but generally even in that file "//"
> is used even for multi-line comments ;)
I saved the "/*" style for the more literary overview comment, and 
converted the shorter, more technical comments to "//" style. I think 
this is pretty common usage (probably due to editor support for "/*" 
comments making it so easy).
>   - the comment is not about (general) -XX argument processing, but
> processing of obsolete and deprecated arguments.
OK, I doubled-down and added more comments about -XX arguments in 
general and more details for the specific cases too (e.g. EXPIRED).
>   - note that not only globals.hpp may contain -XX arguments, but also 
> other files.
I just heard about that as you replied. Noted in comment.
>   - typo: Obosloete -> Obsolete
Yes, this was a typo, but for "Obosloette" - The French word for the 
process of replacing worn out household
objects with small oboes. I'm not sure what I was trying to say with 
that though :-)
>   - not sure if the type name "SpecialFlag" is very descriptive.
Yes, a poor name. In context I think it makes sense enough though, and 
it is only used within this file.
>   - comments always start with upper case (e.g. "// when the
> warning ...") with full stop at the end in the SpecialFlag struct.
OK.  Fixed preexisting comments.
>   - I would prefer to remove the "Declare an" before the description of
> SpecialFlag (or "Describes ..."). It does not seem to add anything. Also
> I would move part of the description of the deprecated_vm_flags to the
> SpecialFlag description, because it seems to describe what a SpecialFlag
> is, and not what the deprecated_jvm_flags table contains.
Yes, makes sense.
>   - maybe add a one-liner what AliasedFlag really is to its description
> instead of speaking generally about what an alias is (which has already
> been done in the large introductory comment).
Yep.
>   - please only describe the interface in the definition of a method
> (i.e. in the hpp file), not in the implementation (cpp file). If you
> have a general implementation specific comment you may put it into the
> cpp file. (.e.g is_newly_obsolete)
OK, gone.
>   - maybe line up the closing braces of the aliased_jvm_flags array too
> like the others
> arguments.hpp:
>   - the comment for is_deprecated_flag() is wrongly indented. Also the
> second sentence has odd additional spaces.
OK.
>   - do not try to repeat the exact same comment for a method in the cpp
> file. Actually the one for is_deprecated_flag() in the cpp file is
> already different and apparently outdated...
OK, Pushed correct comments up to .hpp.
>
>   - extra closing bracket after "(should be ignored)"
>
>   - the comment for handle_aliases_and_deprecation() should start with
> upper case.
>
>   - what is a "real" name of an option argument?
OK.
> TEST.groups:
>
>   - if the copyright for a particular file spans multiple years, the
> official format is "X, Z", not "X, Y, Z"
Yes, that looked odd to me too. I'm glad the correct way looks better.

Thanks again!


On 1/12/15 5:10 PM, Derek White wrote:
> http://cr.openjdk.java.net/~drwhite/8066821/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8066821
>
> This webrev adds support for handling "/deprecated/" -XX options 
> (options that still *do* something but are planned for removal) and 
> "/alias/" options (alternate names for other -XX options) by simply 
> adding entries to the deprecated_jvm_flags and/or aliased_jvm_flags 
> tables. This follows the
> example of the existing obsolete_jvm_flags table.
>
> This replaces a lot of ad-hoc and occasionally wrong code in 
> arguments.cpp (including Arguments::check_deprecated_gc_flags) as well 
> as supporting automatically disabling options after a certain version.
>
> Removed Code:
>  - Removed global DefaultMaxRAMFraction, which was an "improper" alias 
> for "MaxRAMFraction" (two variables that were roughly kept in sync vs. 
> two names for the same variable).
>  - Arguments::check_deprecated_gc_flags().
>  - Alias handling code in Arguments::parse_each_vm_init_arg().
> It also avoids future ad-hoc and occasionally wrong code as new 
> options get aliased and deprecated.
>
> Tests:
> Deprecated and alias options can be tested by adding entries to tables 
> in new tests:
>   VMAliasOptions.java
>   VMDeprecatedOptions.java
>
> The new tests subsume these existing tests:
>   test/gc/startup_warnings/TestDefaultMaxRAMFraction.java
>   test/gc/startup_warnings/TestNoParNew.java
>
>
> Tests run:
>     jprt
>     jtreg
>
> Thanks,
>
>  - Derek

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150115/221c58c7/attachment.htm>