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

[Derek White]

Hi Kim,

On 7/21/15 6:21 PM, Kim Barrett wrote:
> On Jul 17, 2015, at 1:30 PM, Derek White <derek.white at oracle.com> wrote:
>> Request for review:
>>
>> [This updated webrev is being sent to wider audience, and has been merged with Gerard's option constraints check-in. Also factored out -XX:+AggressiveHeap processing into it's own chapter. I mean function :-)]
>>
>> http://cr.openjdk.java.net/~drwhite/8066821/webrev.06/
>> https://bugs.openjdk.java.net/browse/JDK-8066821
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
>   284   JDK_Version obsoleted_in; // When the warning started (obsolete or deprecated).
>
> "obsoleted_in" is confusingly named, given that it covers both the
> "obsolete" and the "deprecated" states.  I think other reviewers
> questioned whether "obsolete" was the proper term for that state.
>
> At the risk of bikeshedding, I did a little exploring with a
> thesaurus, and "discontinued" seems like a possibly better term for
> that state.  The obsoleted_in field could retain that name, as
> covering both deprecation and discontinuation.  Of course, that would
> require some further updates, such as renaming is_newly_obsolete, and
> updating various comments.
>
> Some other possibilities include "defunct" and "disused", but I like
> "discontinued" better than either of those.
I like "discontinued" better than "obsolete", but I think 
"warning_started_in" is more descriptive and doesn't define a new term.
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
>   288 // When a flag is eliminated, it can be added to this list in order to
>
> I think "is eliminated" => "is made obsolete" or something like that.
> I would expect "eliminated" == "removed", which is not what is being
> described here.

OK.
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
>   320 // When a flag is deprecated, it can be added to this list in order to issuing a warning when the flag is used.
>
> "issuing" => "issue"

OK
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
>   808       return false; // "name" is a deprecated option that has expired.
>
> I think the comment is wrong here.  I think it could just be a bogus
> option name.  Later calls don't have any corresponding comment (which
> is fine, just makes this one that I think might be wrong stand out
> more).

OK
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.hpp
>   420   // Returns true if the flag is obsolete and fits into the range specified
>   421   // for being ignored.  In the case the 'version' buffer is filled in with
>   422   // the version number when the flag became obsolete. Otherwise the flag has
>   423   // expired and should be ignored.
>   424   static bool is_newly_obsolete(const char* flag_name, JDK_Version* version);
>
> The "otherwise" part of the description is not correct.  If this
> returns false we don't actually know it has expired.  It could simply
> not be present in the set of obsolete options.

OK
> Also, why the "newly" in the name?  "is_obsolete_flag" would be
> consistent with "is_deprecated_flag".

That was pre-existing. I think they meant that "newly obsolete" options 
should get a warning, but "oldly obsolete" options don't. This webrev 
now defines "expired" to describe the later case though, so yes, maybe I 
should ditch the "newly".


Thanks Kim!

  - Derek