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

David Holmes david.holmes at oracle.com
Tue Sep 1 02:57:47 UTC 2015 

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