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

[Dmitry Dmitriev]

Hi Max,

Thank you fixing that issue! Looks good to me, but I am not a reviewer.

Regards,
Dmitry

On 02.04.2015 21:43, 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 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
>