RFR: 8066821(S) Enhance command line processing to manage deprecating and obsoleting -XX command line arguments
derek.white at oracle.com
Thu Jan 15 23:34:41 UTC 2015
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:
Thank you Thomas for your review.
I lost your email somehow but recovered the text. My comments inline below:
> - in the big comment please avoid adding special headers/footers to
> sections of the file. Over time they just get orphaned and waste up
> - 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).
> - 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)
> - maybe line up the closing braces of the aliased_jvm_flags array too
> like the others
> - the comment for is_deprecated_flag() is wrongly indented. Also the
> second sentence has odd additional spaces.
> - 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?
> - 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.
On 1/12/15 5:10 PM, Derek White wrote:
> 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.
> Deprecated and alias options can be tested by adding entries to tables
> in new tests:
> The new tests subsume these existing tests:
> Tests run:
> - Derek
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-gc-dev