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

[Coleen Phillimore]

Yes, this looks good with the changes that Lois and Dan suggested for 
formatting.
Thanks!
Coleen

On 11/13/14, 6:52 PM, Daniel D. Daugherty wrote:
> Max,
>
> I'm good with this version also.
>
> src/share/vm/runtime/arguments.cpp
>     No content comments; same formatting comments as Lois.
>
>     Background: It is generally faster to do length checks before
>     string comparisons. However, in this case, speed is not an issue.
>
> test/runtime/CommandLine/ObsoleteFlagErrorMessage.java
>     lines 39-41: you should have a space after '//' and before your
>         comment begins for readability.
>
> I don't need to see another code review if you choose to fix any
> of the formatting issues.
>
> Dan
>
>
> On 11/13/14 4:43 PM, Lois Foltan wrote:
>> 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
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>