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

[Daniel D. Daugherty]

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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>