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

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", JDKVersion::undefined(), JDKVersion::jdk(9), JDKVersion::undefined() }, That appears to be obsolete. Did you mean: { "not deprecated or obsolete", JDKVersion::undefined(), JDKVersion::undefined(), JDKVersion::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 isobsolete doesn't care about EXPIRED but isdeprecated does? Because the caller of isobsolete doesn't care about EXPIRED but the caller of isdeprecated does :-) In the first case, Arguments::processargument 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 processargument treats both cases the same (it's in the definition of EXPIRED). In any case, that was the contract for the original isnewlyobsolete() method. In the "isdeprecated" case, the caller (parseargument) 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 parseargument. So parseargument 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 verifyspecialjvmflags -- that will remind us to clean up the flags when the version number changes. So the expiredin 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 deprecatedjvmflags and obsoletejvmflags 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 ("specialjvmflags"). 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 specialjvmflags 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: obsoletejvmflags - 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: deprecatedjvmflags - 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 deprecatedjvmflags and forget to add it to the obsoletejvmflags, as the comments under deprecatedjvmflags 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 deprecatedjvmflags and obsoletejvmflags 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 deprecatedjvmflags, 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 deprecatedjvmflags 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