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

[David Holmes]

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. I'm unclear why is_obsolete 
doesn't care about EXPIRED but is_deprecated does? Also you often state 
something like "fits into the range specified" but it is unclear what 
range you are referring to.

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
>>>>>>>> ....
>