RFR: 8073989: Deprecated integer options are now recognized as obsolete.

[Max Ockner]

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 is_newly_obsolete() 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:process_argument() now strips everything except
>> for the arguments name. (example: -XX:+SomeBooleanFlag ->
>> "SomeBooleanFlag", and -XX:SomeIntegerFlag=100 -> "SomeIntegerFlag")
>> This stripped argument name (fittingly called stripped_argname) is now
>> passed into is_newly_obsolete, 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
>> is_newly_obsolete. If the argument is found to be newly obsolete, the
>> warning message now prints stripped_argname instead of argname to avoid
>> suggesting that "SomeBooleanFlag=100" was ever a supported option.
>
> So previously is_newly_obsolete handled +/- but despite the comment 
> didn't really handle the flag=xxx form. So now before calling 
> is_newly_obsolete 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 is_newly_obsolete calling block, prior to:
>
>  883   Flag* found_flag = Flag::find_flag((const char*)argname, 
> arg_len, true, true);
>  884   if (found_flag != 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:process_argument 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