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 <
revi...@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
>
>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
