[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
Piotr Padlewski via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 15 13:00:40 PST 2016
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
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cfe-commits