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

Derek White derek.white at oracle.com
Tue Sep 1 16:46:33 UTC 2015 

Thanks David!

  - Derek

On 8/31/15 10:57 PM, David Holmes wrote:
> On 1/09/2015 4:29 AM, Derek White wrote:
>> 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?
>
> Nope that is fine thanks.
>
> David
>
>
>>   - 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