Hi,
I think Max's code looks fine with the formatting changes you suggested,
and changes that Lois have suggested. I didn't think your coding
suggestion made it that much clearer really. Or it's 6 of one, half
dozen of the other...
Thanks!
Coleen
On 11/12/14, 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
>>>>
>>>>
>>>
>>
>