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

Derek White derek.white at oracle.com
Mon Aug 31 18:29:09 UTC 2015 

Hi David,

Thanks for the comments.

On 8/31/15 12:12 AM, David Holmes wrote:
> Hi Derek,
>
> Looking good - thanks for the persistence!
>
> One minor comment in arguments.cpp:
>
>  327   { "not deprecated or obsolete",   JDK_Version::undefined(), 
> JDK_Version::jdk(9), JDK_Version::undefined() },
>
> That appears to be obsolete. Did you mean:
>
>  { "not deprecated or obsolete", JDK_Version::undefined(), 
> JDK_Version::undefined(), JDK_Version::jdk(9) },

Yes, that was wrong. I fixed this, retested and verified all expected 
errors were printed. I also moved the test data to the end of the table 
to keep the import stuff first.

I'm re-running jrprt. Would you like another webrev?

  - Derek
>
> Thanks,
> David
>
>
>
> On 28/08/2015 7:53 AM, Derek White wrote:
>> 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