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

[Mads Ravn via cfe-commits]

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/9c7603f0/attachment.html>