RFR: 8073989: Deprecated integer options are now recognized as obsolete. (original) (raw)

David Holmes david.holmes at oracle.com
Fri Apr 3 10:11:16 UTC 2015

Thanks looks good.

David

On 3/04/2015 4:43 AM, Max Ockner wrote:

I have rearranged the misplaced comments. New Webrev: http://cr.openjdk.java.net/~mockner/8073989.3/

I still need one more (r)eviewer. Thanks, Max On 3/30/2015 10:01 PM, David Holmes wrote: Hi Max,

On 31/03/2015 4:08 AM, Max Ockner wrote: Hello all, Please review this change involving the handling of obsolete command line flags.

Bug: https://bugs.openjdk.java.net/browse/JDK-8073989 Webrev: http://cr.openjdk.java.net/~mockner/8073989.2/ Argument processing seems to be a never ending source of bugs. Summary: Three key components to this bug: Thanks for the detailed description! (1) When isnewlyobsolete() checks for "SomeObsoleteIntegerFlag=100" in the flag table, it is not recognized as a match with "SomeObsoleteIntegerFlag" because the lengths are different. This has been fixed. Arguments:processargument() now strips everything except for the arguments name. (example: -XX:+SomeBooleanFlag -> "SomeBooleanFlag", and -XX:SomeIntegerFlag=100 -> "SomeIntegerFlag") This stripped argument name (fittingly called strippedargname) is now passed into isnewlyobsolete, preventing the length check from failing on obsolete (but formeerly valid) arguments. It also eliminates the need for any case work and other string gymnastics from the the body of isnewlyobsolete. If the argument is found to be newly obsolete, the warning message now prints strippedargname instead of argname to avoid suggesting that "SomeBooleanFlag=100" was ever a supported option. So previously isnewlyobsolete handled +/- but despite the comment didn't really handle the flag=xxx form. So now before calling isnewlyobsolete the leading +/- or trailing = is stripped, so only the base argument name is actually checked. Ok. I think these comments were misplaced originally, and seem more so now: 874 // For locked flags, report a custom error message if available. 875 // Otherwise, report the standard unrecognized VM option. they belong after the isnewlyobsolete calling block, prior to: 883 Flag* foundflag = Flag::findflag((const char*)argname, arglen, true, true); 884 if (foundflag != NULL) { (2) Some flags used to be defined in both the flags table and the obsolete flags table. Because of this, those obsolete flags which were also defined in the flags table could be fuzzy matched to provide better error messages. Now that no flag is allowed to be in both tables, it is pointless to attempt to fuzzy match an obsolete flag, since fuzzy matching only looks in the flags table (not the obsolete flags table). The section at the end of Arguments:processargument in which fuzzy matching is attempted on obsolete arguments no longer makes sense and has been removed. Ok. (3) ObsoleteFlagErrorMessage.java has been modified. The existing test for an obsolete flag with appended junk no longer tests for fuzzy matching, and a second test case has been added for integer valued flags. Ok. The test will need to use a new "newly obsolete" flag in JDK 10. :) Reviewed! Thanks, David Tested with jtreg runtime tests. Thanks, Max Ockner

More information about the hotspot-dev mailing list