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

David Holmes david.holmes at oracle.com
Mon Aug 10 05:53:32 UTC 2015

Hi Derek,

I don't have any further comments.


On 7/08/2015 12:46 PM, Derek White wrote:
> Another RFR.
> I've updated based on David and Kim's last comments.
> http://cr.openjdk.java.net/~drwhite/8066821/webrev.07/
> https://bugs.openjdk.java.net/browse/JDK-8066821
> I also have a webrev of webrev.06 vs. webrev.07:
> http://cr.openjdk.java.net/~drwhite/8066821/webrev.6.v.7/
> [This webrev is confused about CommandLineOptionTest.java. I
> double-checked with a recursive diff and the only differences are in
> arguments.cpp and arguments.hpp.]
> Thanks for looking!
>   - Derek
> On 7/23/15 12:09 PM, Derek White wrote:
>> On 7/20/15 10:03 PM, David Holmes wrote:
>>> On 21/07/2015 3:02 AM, Derek White wrote:
>>>> Hi David,
>>>> Thanks for looking this over.
>>>> On 7/20/15 5:29 AM, David Holmes wrote:
>>>>> Hi Derek,
>>>>> Sorry but I'm finding this a bit confused and inconsistent. Comments
>>>>> below.
>>>>> On 18/07/2015 3:30 AM, Derek White 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
>>>>> argument.cpp:
>>>>>  258  *               been not scheduled).
>>>>> -> not been scheduled
>>>> OK.
>>>>>  260 *     OBSOLETE: An option may be removed (and deleted from
>>>>> globals.hpp), but still be accepted on the command
>>>>>  261  *               line. A warning is printed to let the user know
>>>>> that support may be removed in the future.
>>>>> Isn't this the stronger case that support has already been removed
>>>>> (the option does nothing) and it will be removed at the next major
>>>>> release. At the start of a major release we should be able to delete
>>>>> all entries from the obsolete table - else it wasn't obsolete but
>>>>> deprecated.
>>>>> Not sure "obsolete" is the right term here - obsolete things still
>>>>> function. For us an obsolete option does nothing (it exists only in
>>>>> the obsolete table).
>>>> It's not a great name, but that what the previous code called it.
>>>> It's a
>>>> "I'll pretend you didn't say that" option, like when a teenager
>>>> hears an
>>>> adult use out-of-date slang ("groovy!"). How about a "condescending"
>>>> option :-)
>>> Name aside you didn't comment on my main comment here: an obsolete
>>> option has already been removed from globals etc and does nothing.
>> Ahh, OK. I must have been confusing in tense. I'll rewrite to be more
>> direct.
>>>>>  276  * Tests:  Aliases are tested in VMAliasOptions.java.
>>>>>  277  *         Deprecated options are tested in
>>>>> VMDeprecatedOptions.java.
>>>>>  278  *         Obsolete options are tested in various files.
>>>>> We don't normally document this kind of thing in the source.
>>>> I'm trying to head off unnecessary one-off test files for each alias
>>>> and
>>>> deprecated option. For example TestNoParNew.java and
>>>> TestDefaultMaxRAMFraction.java. And I think that there should be one
>>>> test file for obsolete options also (perhaps expanding
>>>> ObsoleteFlagErrorMessage.java), instead of a bunch of separate test
>>>> files, but that is not in this webrev.
>>> Sounds fine but again we don't normally document test strategies in
>>> the source code.
>> Normally I'd agree but I'm trying to change current practice of tests
>> popping up on the corpses of dead or dying options like mushrooms in a
>> forest.  I'm doubly adamant if I can add that last sentence to the
>> comment :-)
>>>>>  281 // Obsolete or deprecated -XX flag.
>>>>> I can tell this is going to get confusing already.
>>>>>  284   JDK_Version obsoleted_in; // When the warning started (obsolete
>>>>> or deprecated).
>>>>> But there is a difference: deprecated == warn but still process;
>>>>> obsolete == warn and ignore.
>>>> Yes, but the SpecialFlag type is concerned with the common aspect of
>>>> warning. The "ignore" or "process" aspects are handled by the different
>>>> functions that look up the obsolete_jvm_flags and deprecated_jvm_flags
>>>> arrays.
>>> So that variable should really be obsoleted_or_deprecated_in ?
>> OK, I see now. It was right in front of me. Maybe "warning_started_in"
>> is simpler.
>>>>>  288 // When a flag is eliminated, it can be added to this list in
>>>>> order to
>>>>>  289 // continue accepting this flag on the command-line, while
>>>>> issuing a warning
>>>>>  290 // and ignoring the value.  Once the JDK version reaches the
>>>>> 'accept_until'
>>>>>  291 // limit, we flatly refuse to admit the existence of the flag.
>>>>>  292 static SpecialFlag const obsolete_jvm_flags[] = {
>>>>> When a flag is eliminated it is gone from this table. As soon as the
>>>>> accept_until version is the current version we wipe the table of all
>>>>> such entries. This should be one of the first things done in a new
>>>>> release.
>>>> I completely agree that this is a great plan. But until this April we
>>>> still had obsolete flags listed for JDK 5! The obsolete_jvm_flags table
>>>> did the right thing until the process caught up. In any case, this
>>>> webrev doesn't really change the behavior of the obsolete_jvm_flags
>>>> table.
>>> But what you are seeing before April is the result of hotspot express
>>> (at least in a large part). Now that we don't have to support that we
>>> don't have legacy lists to maintain. The code could even be changed
>>> upon detecting that current JDK version == "until" version to report
>>> "Hey you forgot to update the table!" ;-)
>> OK, that history makes more sense.
>>> My point is that the comments should reflect how we intend to use
>>> these, not give the impression that keeping eliminated options in the
>>> table is the expected thing to do.
>> OK, I'll update the comments both here and up above to describe the
>> presumably common "deprecated"->"obsolete" lifecycle.
>>>>> 320 // When a flag is deprecated, it can be added to this list in
>>>>> order to issuing a warning when the flag is used.
>>>>>  321 // Once the JDK version reaches the 'accept_until' limit, we
>>>>> flatly refuse to admit the existence of the flag.
>>>>>  322 static SpecialFlag const deprecated_jvm_flags[] = {
>>>>> A deprecated flag should be obsoleted before it reaches the
>>>>> accept_until limit.
>>>> That's a policy that's under discussion on hotspot-runtime-dev. There
>>>> are certainly option lifecycles that have been approved by our internal
>>>> process that don't follow this proposed policy. The mechanism in this
>>>> webrev was concerned about supporting the plethora of current
>>>> lifecycles, for better or worse.
>>> But again comments should reflect expected usage - if we reach the
>>> "until" version of a deprecated option that wasn't obsoleted then
>>> something has probably gone wrong.
>> OK.
>>>>> Which suggests that when we start a new version we wipe the obsoleted
>>>>> table and move the deprecated options into it.
>>>> I can add documentation to describe this case.
>>>> If we decide that we'll always treat a deprecated or aliased option as
>>>> obsolete for one extra release, then it might make sense to have a
>>>> combined option lifecycle table. But for now I like the fact that
>>>> options in deprecated_jvm_flags should always have a real variable
>>>> defined in globals.hpp (etc), and options in obsolete_jvm_flags should
>>>> never have a global variable.
>>> Yes I like that too.
>>>>> 776 case 1: {
>>>>>  777       if (warn) {
>>>>>  778         char version[256];
>>>>>  779         since.to_string(version, sizeof(version));
>>>>>  780         if (real_name != arg) {
>>>>>  781           warning("Option %s was deprecated in version %s and
>>>>> will likely be removed in a future release. Use option %s instead.",
>>>>>  782                   arg, version, real_name);
>>>>>  783         } else {
>>>>>  784           warning("Option %s was deprecated in version %s and
>>>>> will likely be removed in a future release.",
>>>>>  785                   arg, version);
>>>>>  786         }
>>>>> This isn't distinguishing between deprecated and obsoleted ???
>>>> Yes, handle_aliases_and_deprecation() doesn't handle obsolete options
>>>> (or it would have had a longer name :-) Maybe it should handle that
>>>> case, but it would have complicated the control flow in the caller. I
>>>> have another proposed change in the works that simplifies the caller
>>>> quite a bit that would make the refactoring simpler.
>>> I need to think more on that. It is hard to see the overall control
>>> flow for argument processing.
>> Yes it is hard to see. Currently options are parsed into tokens in a
>> couple of places. It's roughly either:
>>   [+][-]arg
>> or:
>>   arg[:]=[value]
>> But not all of the code handles ":=". Furthermore option processing
>> figures out the type of the argument by repeatedly trying to scanf
>> "value" to see if it looks like floating point, or an integer, or
>> maybe just text. I think we should first parsing out the arg name, get
>> the Flag, and ask the Flag what type it is. This would make it much
>> easier to handle aliases, deprecation, and obsoleting options in one
>> place.
>> I have a workspace that does this but I suspect there could be slight
>> differences in how errors would be reported.
>>> Thanks,
>>> David
>> Thanks for looking at this again. It's heading in the right direction!
>>  - Derek

More information about the hotspot-dev mailing list