[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Piotr Padlewski via cfe-commits]

I think that the feature I mentioned is right thing to put in this check,
however you don't have to implement it right now, just leave FIXIT comment

2016-12-15 20:55 GMT+01:00 Mads Ravn <madsravn at gmail.com>:

> Hi Piotr,
>
> That is a good point. Because it is not always -1 or 1 that determines
> lexicographical higher or lower.
>
> However, I don't think that is in the scope of this check. This check
> checks for string comparison (equality or inequality). Adding a match for
> if the user is using the compare function semantically wrong might make the
> check too ambiguous. Since str.compare(str) == 0 will check for equality
> and str.compare(str) != 0 will check for inequality. But str.compare(str)
> == 1 will check whether one string is lexicographically smaller than the
> other (and == -1 also). What do you think?
>
> Best regards,
> Mads Ravn
>
> On Thu, Dec 15, 2016 at 8:17 PM Piotr Padlewski via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> Prazek added a comment.
>>
>> Good job.
>> I think it is resonable to warn in cases like:
>>
>>   if (str.compare(str2) == 1)
>>
>> or even
>>
>>   if(str.compare(str2) == -1)
>>
>> Sometimes people check for 1 or -1 instead of > 0 and < 0. If I remember
>> corectly PVS studio found some bugs like this.
>>
>>
>>
>> ================
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:27
>> +                               hasName("::std::basic_string"))))),
>> +      hasArgument(0, declRefExpr()), callee(memberExpr()));
>> +
>> ----------------
>> malcolm.parsons wrote:
>> > I don't think you need to check what the first argument is.
>> +1, just check if you are calling function with 1 argument.
>> you can still use hasArgument(0, expr().bind("str2")) to bind argument
>>
>>
>> ================
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
>> +    return;
>> +  const auto strCompare = cxxMemberCallExpr(
>> +      callee(cxxMethodDecl(hasName("compare"),
>> ----------------
>> Start with upper case
>>
>>
>> https://reviews.llvm.org/D27210
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161215/9cb58753/attachment-0001.html>