etienneb updated this revision to Diff 55240.
etienneb marked an inline comment as done.
etienneb added a comment.
address alexfh comments, add a macro FP.
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,32 @@
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;
+}
+
+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 +361,82 @@
return 1;
}
+
+#define uprv_checkValidMemory(a, b) ((void)(a))
+#define uprv_strncmp(s1, s2, n) ( \
+ uprv_checkValidMemory(s1, 1), \
+ uprv_checkValidMemory(s2, 1), \
+ strncmp(s1, s2, n))
+
+int strncmp_macro_uprv(const char* a, const char* b) {
+ if (uprv_strncmp(a, b, 4))
+ return 0;
+ // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strncmp' is called without explicitly comparing result
+
+ if (uprv_strncmp(a, b, 4) == 2)
+ return 0;
+ // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strncmp' is compared to a suspicious constant
+
+ if (uprv_strncmp(a, b, 4) <= .0)
+ return 0;
+ // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strncmp' has suspicious implicit cast
+
+ if (uprv_strncmp(a, b, 4) + 0)
+ return 0;
+ // CHECK-MESSAGES: [[@LINE-2]]:7: warning: results of function 'strncmp' used by operator '+'
+
+ 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,43 @@
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(
+ ofClass(hasName("::llvm::StringRef")),
+ hasAnyName("compare", "compare_lower", "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))));
+
+ // Match common macros used to wrapped runtime functions.
+ const auto CondMacroStringCompareCallExpr = conditionalOperator(anyOf(
+ hasTrueExpression(ignoringParenImpCasts(DirectStringCompareCallExpr)),
+ hasFalseExpression(ignoringParenImpCasts(DirectStringCompareCallExpr))));
+ const auto CommaMacroStringCompareCallExpr = binaryOperator(
+ hasOperatorName(","),
+ hasRHS(ignoringParenImpCasts(DirectStringCompareCallExpr)));
+
// The implicit cast is not present in C.
const auto StringCompareCallExpr = ignoringParenImpCasts(
- anyOf(DirectStringCompareCallExpr, MacroStringCompareCallExpr));
+ anyOf(DirectStringCompareCallExpr, CondMacroStringCompareCallExpr,
+ CommaMacroStringCompareCallExpr, StdStringCompareCallExpr,
+ LLVMStringCompareCallExpr));
if (WarnOnImplicitComparison) {
// Detect suspicious calls to string compare:
@@ -155,23 +178,34 @@
// Detect suspicious operator with string compare function as operand.
Finder->addMatcher(
- binaryOperator(
- unless(anyOf(matchers::isComparisonOperator(), hasOperatorName("&&"),
- hasOperatorName("||"), hasOperatorName("="))),
- hasEitherOperand(StringCompareCallExpr))
+ binaryOperator(unless(anyOf(matchers::isComparisonOperator(),
+ hasOperatorName("&&"), hasOperatorName("||"),
+ hasOperatorName("="), hasOperatorName(","))),
+ hasEitherOperand(StringCompareCallExpr))
.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 +245,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
