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

Derek White derek.white at oracle.com
Thu Aug 27 21:53:12 UTC 2015 

Hi David,

Comments inline...

On 8/27/15 12:55 AM, David Holmes wrote:
> On 26/08/2015 8:02 AM, Derek White wrote:
>> Thanks Karen!
>>
>> If I don't get any other feedback, can I get you to sponsor the change
>> for me? After folding in your fixes...
>
> I need to see those fixes first as I got quite confused trying to 
> reconcile the code with the documentation.
OK, I've integrated Karen's final documentation feedback.

> I'm unclear why is_obsolete doesn't care about EXPIRED but 
> is_deprecated does? 
Because the caller of is_obsolete doesn't care about EXPIRED but the 
caller of is_deprecated does :-)

In the first case, Arguments::process_argument has already tried 
processing the argument as a normal, aliased, or deprecated flag, and 
failed. Right before it starts whining about illegal flags it checks to 
see if it's obsolete, and if so prints the warning and continues on to 
the next flag. If the flag isn't obsolete then it must be non-existent. 
But process_argument treats both cases the same (it's in the definition 
of EXPIRED). In any case, that was the contract for the original 
is_newly_obsolete() method.

In the "is_deprecated" case, the caller (parse_argument) is in the 
middle of handling a real-life global flag, but it needs to decide if 
should print a deprecation warning, or treat it as a normal flag, or 
note that the flag is now obsolete or expired, in which case it has to 
bail out of parse_argument. So parse_argument needs to know three states.
> Also you often state something like "fits into the range specified" 
> but it is unclear what range you are referring to.

I simplified both the code and documentation you mentioned. The 
implementations had gotten "overly factored". I hope you find the new 
code and documentation clearer.

I have a JPRT build out running, but are the new webrevs:

    http://cr.openjdk.java.net/~drwhite/8066821/webrev.09
    http://cr.openjdk.java.net/~drwhite/8066821/webrev.8.v.9
    http://cr.openjdk.java.net/~drwhite/8066821/webrev.7.v.9

Thanks for your comments!

  - Derek
>
> Thanks,
> David
>
>>   - Derek
>>
>> On 8/25/15 2:05 PM, Karen Kinnear wrote:
>>> Derek,
>>>
>>> Many thanks for the latest changes. And thank you for the
>>> verify_special_jvm_flags -- that will remind us to
>>> clean up the flags when the version number changes. So the expired_in
>>> information is for internal use only - we
>>> don't print that information for users, we simply tell them "in a
>>> future release".
>>>
>>> Looks good. No need for my further code review.
>>>
>>> Minor comments:
>>> 1. arguments.hpp
>>> line 421 "In the case" -> "In this case"
>>>
>>> 2. arguments.cpp comments
>>>
>>> I love the high level comments - could you possibly update them?
>>>     - e.g. they still talk about the deprecated_jvm_flags and
>>> obsolete_jvm_flags tables.
>>>     - e.g. expired and obsolete options should have their global
>>> variable definitions and
>>>       related implementations removed ...
>>>
>>> comment line 2119 - extra spaces at the front? (or webrev artifact?)
>>>
>>> thanks very much,
>>> Karen
>>>
>>>
>>>
>>>
>>> On Aug 24, 2015, at 1:27 PM, Derek White wrote:
>>>
>>>> RFR:
>>>>
>>>> This version incorporates Dave and Karen's suggestions to
>>>> automatically handle deprecated-> obsolete transitions. This is
>>>> implemented with a unified table of deprecated and obsolete options
>>>> ("special_jvm_flags").
>>>>
>>>> Karen also suggested (offline) that it would be good to verify that
>>>> obsolete and expired options no longer have a "globals" variable
>>>> defined. This revision adds debug checking of the special_jvm_flags
>>>> table to check a bunch of constraints, such as deprecated version
>>>> must be < obsolete version < expired version, and checks for
>>>> duplicate table entries as well as looking for left-over "globals"
>>>> variables.
>>>>
>>>> http://cr.openjdk.java.net/~drwhite/8066821/webrev.08
>>>> https://bugs.openjdk.java.net/browse/JDK-8066821
>>>>
>>>> I also have a webrev of webrev.07 vs. webrev.08:
>>>> http://cr.openjdk.java.net/~drwhite/8066821/webrev.7.v.8
>>>>
>>>> Thanks for looking!
>>>>
>>>> - Derek
>>>>
>>>> On 8/11/15 3:33 PM, Derek White wrote:
>>>>> Hi Karen,
>>>>>
>>>>> As much as I want to get this RFE over and out, I see your and David
>>>>> H's point :-)
>>>>>
>>>>> I'll spin another rev...
>>>>>
>>>>> - Derek
>>>>>
>>>>> On 8/10/15 6:07 PM, Karen Kinnear wrote:
>>>>>> Derek,
>>>>>>
>>>>>> Thank you for all of your work on this and responses to suggestions.
>>>>>>
>>>>>> I had a couple of questions/comments:
>>>>>>
>>>>>> I appreciate that you have created mechanisms to allow:
>>>>>> 1. 2 step obsolete: obsolete_jvm_flags
>>>>>>      - step 1: obsolete: warn and ignore
>>>>>>      - step 2: expire
>>>>>>      - and I expect we will use this for most -XX flags, especially
>>>>>> develop flags
>>>>>>
>>>>>> 2. 2 step deprecate: deprecated_jvm_flags
>>>>>>      - step 1: deprecate: warn and handle
>>>>>>      - step 2: expire
>>>>>>
>>>>>> I see this approach as useful for aliasing and for a few other
>>>>>> flags, but not
>>>>>> the common path.
>>>>>>
>>>>>> My concerns:
>>>>>>
>>>>>> With my understanding of how this is set up today, for the flags
>>>>>> that are most exposed to
>>>>>> customers, which would want the 3 step deprecation, it would be too
>>>>>> easy for an engineer
>>>>>> to add the flag to the deprecated_jvm_flags and forget to add it to
>>>>>> the obsolete_jvm_flags,
>>>>>> as the comments under deprecated_jvm_flags suggest.  And we don't
>>>>>> want folks to have
>>>>>> to go back and do a second step later for flags if they could do it
>>>>>> all at once.
>>>>>>
>>>>>> I think it would be confusing to add a flag to both
>>>>>> deprecated_jvm_flags and obsolete_jvm_flags
>>>>>> at the same time, since then the expiration release is not clear.
>>>>>>
>>>>>> 3. For product non-XX, commercial and common -XX flags what I would
>>>>>> like to see is
>>>>>> a three-step table which I would have called deprecated_jvm_flags,
>>>>>> which did
>>>>>>      - step 1: deprecate: warn and handle
>>>>>>      - step 2: obsolete: warn and ignore
>>>>>>      - step 3: expire: unrecognized error
>>>>>>
>>>>>> Would you be willing to add a table like that which is the
>>>>>> recommended approach
>>>>>> for external supported flags?
>>>>>>
>>>>>> So one way to do this would be to change the current
>>>>>> deprecated_jvm_flags to have
>>>>>> three fields, so that the usual path would be to have three
>>>>>> releases listed, and
>>>>>> for aliased flags leave the middle field with the undefined version.
>>>>>>
>>>>>> Another approach would be to have the three fields and have it 
>>>>>> used by
>>>>>> all three approaches, just fill in the steps that are appropriate.
>>>>>>
>>>>>> minor comment:
>>>>>>
>>>>>>
>>>>>> 1. arguments.hpp
>>>>>> Could you possibly change the comments from "and has expired
>>>>>> (should be ignored)" to
>>>>>> "and has expired (unrecognized error)"
>>>>>>
>>>>>> unless what you mean really is should be ignored, which is not the
>>>>>> same expired.
>>>>>>
>>>>>> thanks,
>>>>>> Karen
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Aug 10, 2015, at 12:41 PM, Derek White wrote:
>>>>>>
>>>>>>> Thanks David!
>>>>>>>
>>>>>>> - Derek
>>>>>>>
>>>>>>> On 8/10/15 1:53 AM, David Holmes wrote:
>>>>>>>> Hi Derek,
>>>>>>>>
>>>>>>>> I don't have any further comments.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>> 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
>>>>>>>>> ....
>>

More information about the hotspot-dev mailing list