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 31 04:12:51 UTC 2015 

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) },

?

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