RFR:8060449:Proper error messages for newly obsolete command line flags.

[Lois Foltan]

Hi Max,

This looks good!  Three really minor coding style comments included for 
completeness but I don't need to see another code review if you choose 
to fix these.

src/share/vm/runtime/arguments.cpp
   - line # 336, usually the { would be placed on line 335 at the end of 
the if statement's conditional expression
   - line # 945, need a blank space between the "){"
   - line #952 the closing } is not lined up with the if keyword

Again, these are minor.
Lois


On 11/13/2014 6:06 PM, Max Ockner wrote:
> Correction - new webrev is at 
> http://cr.openjdk.java.net/~coleenp/8060449.1/
>
> Max Ockner
>
>
> On 11/12/2014 3:38 PM, Daniel D. Daugherty wrote:
>> On 11/12/14 1:04 PM, Max Ockner wrote:
>>> Dan,
>>> I have reformatted  the  "){" fragment on line 336 as you 
>>> recommended. Thanks for catching that.
>>
>> Thanks.
>>
>>
>>> For your second recommendation, I think I have a use case where the 
>>> recommended code would not function properly:
>>>
>>> Let's say there is a boolean flag SomeFlag, and let's say that the 
>>> user tries to type  "-XX:SomeFlagg".
>>>
>>> The first if statement passes because strlen("SomeFlagg") = 
>>> strlen("SomeFlag")+1.
>>> The second conditional checks if (strncmp(flag_status.name, s, 
>>> f_len) == 0).  But f_len, the length of "SomeFlag" is 8. The result 
>>> is that the 9th character of the user's input, which is where s 
>>> differs from flag_status.name, is not checked,so this condition is 
>>> passed as well.
>>
>> Your use case catches a bug in what I posted. I had originally
>> planned to change the two strncmp() calls to strcmp() so that
>> we get a complete match, but then I couldn't remember if a
>> straight strcmp() triggers Parfait warnings so I couldn't
>> finish reasoning my way through that maze...
>>
>> Switching the 'f_len' parameter to 's_len' would solve the
>> problem without triggering Parfait, but it is totally your
>> call.
>>
>> Dan
>>
>>>
>>> Thanks,
>>> Max
>>>
>>>
>>>
>>> On 11/12/2014 1:41 PM, Daniel D. Daugherty wrote:
>>>> On 11/7/14 12:13 PM, Max Ockner wrote:
>>>>> ID: 8060449
>>>>> webrev: http://cr.openjdk.java.net/~coleenp/8060449/
>>>>
>>>> src/share/vm/runtime/arguments.cpp
>>>>
>>>>     line 336: ) {
>>>>         This fragment is on a line by itself and far left.
>>>>         Minimally, it should align like this:
>>>>
>>>>         line 331:     if (...
>>>>         line 336:        ) {
>>>>
>>>>     However, I recommend a slightly different structure to
>>>>     this logic:
>>>>
>>>>     size_t f_len = strlen(flag_status.name);
>>>>     size_t s_len = strlen(s);
>>>>     if (f_len == s_len || (f_len + 1) == s_len) {
>>>>       // this flag is the right length for a possible match
>>>>       if (strncmp(flag_status.name, s, f_len) == 0) ||
>>>>           ((s[0] == '+' || s[0] == '-') &&
>>>>            strncmp(flag_status.name, &s[1], f_len) == 0)) {
>>>>         // this flag is an exact match
>>>>         if 
>>>> (JDK_Version::current().compare(flag_status.accept_until) == -1) {
>>>>         ...
>>>>         }
>>>>       }
>>>>     }
>>>>     i++;
>>>>
>>>> I have no idea if the above formatting is going to be
>>>> preserved by e-mail clients...
>>>>
>>>> Dan
>>>>
>>>>
>>>>>
>>>>> Summary: A "newly obsolete" command line option is one which is no 
>>>>> longer supported, but still is acknowledged. There is a list of 
>>>>> these in arguments.cpp.
>>>>> It used to be that only a fixed number of characters were checked 
>>>>> when comparing a given command line option to the list of obsolete 
>>>>> flags (strncmp was used, where the number of characters to check 
>>>>> is equal to the length of the flag name from the table.)
>>>>> As a result, an arbitrary string appended to the end of an 
>>>>> obsolete argument goes unnoticed.
>>>>> This issue is fixed by comparing the lengths of the given flag and 
>>>>> the flags from the obsolete flags table.
>>>>> When a misspelled flag is fuzzy-matched to an obsolete flag, an 
>>>>> appropriate warning is given to save the user a few key strokes: 
>>>>> (1) unrecognized option [bad option]. (2) Did you mean [option]? 
>>>>> (3) [option] is obsolete as of [version])
>>>>>
>>>>> A new test for this feature checks for the presence of all three 
>>>>> components of the above error message.
>>>>>
>>>>> Tested with: vm.quick.testlist
>>>>>              hotspot jtreg tests
>>>>>              jprt
>>>>>
>>>>> Thanks for your help!
>>>>> Max Ockner
>>>>>
>>>>>
>>>>
>>>
>>
>