etienneb created this revision.
etienneb added a reviewer: alexfh.
etienneb added a subscriber: cfe-commits.
This patch added the support for method "compare" on string-like classes.
The LLVM stringRef is supported. The checker assume that StringRef is returning
-1, 0 or 1.
Which is not the case for other functions returning <0, 0 or >0.
http://reviews.llvm.org/D19577
Files:
clang-tidy/misc/SuspiciousStringCompareCheck.cpp
test/clang-tidy/misc-suspicious-string-compare.cpp
Index: test/clang-tidy/misc-suspicious-string-compare.cpp
===================================================================
--- test/clang-tidy/misc-suspicious-string-compare.cpp
+++ test/clang-tidy/misc-suspicious-string-compare.cpp
@@ -6,6 +6,34 @@
typedef __SIZE_TYPE__ size;
+namespace std {
+template <typename T>
+class allocator {};
+template <typename T>
+class char_traits {};
+template <typename C, typename T, typename A>
+struct basic_string {
+ typedef basic_string<C, T, A> _Type;
+ basic_string();
+ basic_string(const C *p, const A &a = A());
+
+ int compare(const C* s) const;
+};
+
+typedef basic_string<char, std::char_traits<char>, std::allocator<char> > string;
+typedef basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > wstring;
+}
+
+namespace llvm {
+struct StringRef {
+ StringRef();
+ StringRef(const char*);
+ int compare(StringRef RHS);
+ int compare_lower(StringRef RHS);
+ int compare_numeric(StringRef RHS);
+};
+}
+
struct locale_t {
void* dummy;
} locale;
@@ -335,3 +363,56 @@
return 1;
}
+
+int test_string_patterns() {
+ std::string str;
+ if (str.compare("a"))
+ return 0;
+ // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is called without explicitly comparing result
+ // CHECK-FIXES: if (str.compare("a") != 0)
+
+ if (str.compare("a") == 0 ||
+ str.compare("b"))
+ return 0;
+ // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is called without explicitly comparing result
+ // CHECK-FIXES: str.compare("b") != 0)
+
+ if (str.compare("a") == 1)
+ return 0;
+ // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is compared to a suspicious constant
+
+ if (str.compare("a") == 2)
+ return 0;
+ // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is compared to a suspicious constant
+}
+
+int test_llvm_patterns() {
+ llvm::StringRef str;
+ if (str.compare("a"))
+ return 0;
+ // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is called without explicitly comparing result
+ // CHECK-FIXES: if (str.compare("a") != 0)
+
+ if (str.compare_lower("a"))
+ return 0;
+ // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare_lower' is called without explicitly comparing result
+ // CHECK-FIXES: if (str.compare_lower("a") != 0)
+
+ if (str.compare("a") == 0 ||
+ str.compare("b"))
+ return 0;
+ // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is called without explicitly comparing result
+ // CHECK-FIXES: str.compare("b") != 0)
+
+ if (str.compare("a") == 2)
+ return 0;
+ // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is compared to a suspicious constant
+
+ // The following calls are valid.
+ if (str.compare("a") == 1) return 0;
+ if (str.compare("a") != 1) return 0;
+ if (str.compare("a") == 0) return 0;
+ if (str.compare("a") != 0) return 0;
+ if (str.compare("a") == -1) return 0;
+ if (str.compare("a") != -1) return 0;
+}
Index: clang-tidy/misc/SuspiciousStringCompareCheck.cpp
===================================================================
--- clang-tidy/misc/SuspiciousStringCompareCheck.cpp
+++ clang-tidy/misc/SuspiciousStringCompareCheck.cpp
@@ -8,10 +8,10 @@
//===----------------------------------------------------------------------===//
#include "SuspiciousStringCompareCheck.h"
+#include "../utils/Matchers.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h"
-#include "../utils/Matchers.h"
using namespace clang::ast_matchers;
@@ -106,20 +106,37 @@
ParseFunctionNames(KnownStringCompareFunctions, &FunctionNames);
ParseFunctionNames(StringCompareLikeFunctions, &FunctionNames);
- // Match a call to a string compare functions.
+ // Match a std::string::compare call.
+ const auto StdStringCompareCallExpr =
+ cxxMemberCallExpr(callee(cxxMethodDecl(ofClass(hasName("basic_string")),
+ hasName("compare"))
+ .bind("decl")))
+ .bind("call");
+
+ // Match llvm strings variants.
+ const auto LLVMStringCompareCallExpr =
+ cxxMemberCallExpr(
+ callee(cxxMethodDecl(hasAnyName("::llvm::StringRef::compare",
+ "::llvm::StringRef::compare_lower",
+ "::llvm::StringRef::compare_numeric"))
+ .bind("decl")))
+ .bind("call");
+
+ // Match a call to a string compare functions (i.e. strcmp).
const auto FunctionCompareDecl =
functionDecl(hasAnyName(std::vector<StringRef>(FunctionNames.begin(),
FunctionNames.end())))
.bind("decl");
const auto DirectStringCompareCallExpr =
callExpr(hasDeclaration(FunctionCompareDecl)).bind("call");
- const auto MacroStringCompareCallExpr =
- conditionalOperator(
- anyOf(hasTrueExpression(ignoringParenImpCasts(DirectStringCompareCallExpr)),
- hasFalseExpression(ignoringParenImpCasts(DirectStringCompareCallExpr))));
+ const auto MacroStringCompareCallExpr = conditionalOperator(anyOf(
+ hasTrueExpression(ignoringParenImpCasts(DirectStringCompareCallExpr)),
+ hasFalseExpression(ignoringParenImpCasts(DirectStringCompareCallExpr))));
+
// The implicit cast is not present in C.
const auto StringCompareCallExpr = ignoringParenImpCasts(
- anyOf(DirectStringCompareCallExpr, MacroStringCompareCallExpr));
+ anyOf(DirectStringCompareCallExpr, MacroStringCompareCallExpr,
+ StdStringCompareCallExpr, LLVMStringCompareCallExpr));
if (WarnOnImplicitComparison) {
// Detect suspicious calls to string compare:
@@ -162,16 +179,27 @@
.bind("suspicious-operator"),
this);
- // Detect comparison to invalid constant: 'strcmp() == -1'.
+ // Detect comparison to invalid constant: 'strcmp() == -2'.
const auto InvalidLiteral = ignoringParenImpCasts(
anyOf(integerLiteral(unless(equals(0))),
unaryOperator(hasOperatorName("-"),
has(integerLiteral(unless(equals(0))))),
characterLiteral(), cxxBoolLiteral()));
+ // A subset of functions allow comparison to 1 and -1.
+ const auto ValidLiteral = ignoringParenImpCasts(anyOf(
+ integerLiteral(equals(1)),
+ unaryOperator(hasOperatorName("-"), has(integerLiteral(equals(1))))));
+
+ const auto ValidComparator =
+ binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ hasEitherOperand(LLVMStringCompareCallExpr),
+ hasEitherOperand(ValidLiteral));
+
Finder->addMatcher(binaryOperator(matchers::isComparisonOperator(),
hasEitherOperand(StringCompareCallExpr),
- hasEitherOperand(InvalidLiteral))
+ hasEitherOperand(InvalidLiteral),
+ unless(ValidComparator))
.bind("invalid-comparison"),
this);
}
@@ -211,7 +239,8 @@
<< Decl;
}
- if (const auto* BinOp = Result.Nodes.getNodeAs<BinaryOperator>("suspicious-operator")) {
+ if (const auto *BinOp =
+ Result.Nodes.getNodeAs<BinaryOperator>("suspicious-operator")) {
diag(Call->getLocStart(), "results of function %0 used by operator '%1'")
<< Decl << BinOp->getOpcodeStr();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
