Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -28,6 +28,7 @@ SizeofContainerCheck.cpp SizeofExpressionCheck.cpp StaticAssertCheck.cpp + StringCompareCheck.cpp StringConstructorCheck.cpp StringIntegerAssignmentCheck.cpp StringLiteralWithEmbeddedNulCheck.cpp Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -35,6 +35,7 @@ #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" #include "StaticAssertCheck.h" +#include "StringCompareCheck.h" #include "StringConstructorCheck.h" #include "StringIntegerAssignmentCheck.h" #include "StringLiteralWithEmbeddedNulCheck.h" @@ -105,6 +106,7 @@ CheckFactories.registerCheck( "misc-sizeof-expression"); CheckFactories.registerCheck("misc-static-assert"); + CheckFactories.registerCheck("misc-string-compare"); CheckFactories.registerCheck( "misc-string-constructor"); CheckFactories.registerCheck( Index: clang-tidy/misc/StringCompareCheck.h =================================================================== --- clang-tidy/misc/StringCompareCheck.h +++ clang-tidy/misc/StringCompareCheck.h @@ -0,0 +1,36 @@ +//===--- StringCompareCheck.h - clang-tidy-----------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef STRING_COMPARE_CHECK_H +#define STRING_COMPARE_CHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// This check flags all calls compare when used to check for string +/// equality or inequality. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare-check.html +class StringCompareCheck : public ClangTidyCheck { +public: + StringCompareCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // STRING_COMPARE_CHECK_H Index: clang-tidy/misc/StringCompareCheck.cpp =================================================================== --- clang-tidy/misc/StringCompareCheck.cpp +++ clang-tidy/misc/StringCompareCheck.cpp @@ -0,0 +1,71 @@ +//===--- MiscStringCompare.cpp - clang-tidy--------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "StringCompareCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +void StringCompareCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + const auto strCompare = cxxMemberCallExpr( + callee(cxxMethodDecl(hasName("compare"), + ofClass(classTemplateSpecializationDecl( + hasName("::std::basic_string"))))), + hasArgument(0, declRefExpr()), callee(memberExpr())); + + // First case: if(str.compare(str)) + Finder->addMatcher( + ifStmt(hasCondition(implicitCastExpr( + hasImplicitDestinationType(isInteger()), has(strCompare)))) + .bind("match"), + this); + + // Second case: if(!str.compare(str)) + Finder->addMatcher(ifStmt(hasCondition(unaryOperator( + hasOperatorName("!"), + hasUnaryOperand(implicitCastExpr( + hasImplicitDestinationType(isInteger()), + has(strCompare)))))) + .bind("match"), + this); + + // Third case: if(str.compare(str) == 0) + Finder->addMatcher( + ifStmt(hasCondition(binaryOperator(hasOperatorName("=="), + hasEitherOperand(strCompare), + hasEitherOperand(integerLiteral(equals(0)))))) + .bind("match"), + this); + + // Fourth case: if(str.compare(str) != 0) + Finder->addMatcher( + ifStmt(hasCondition(binaryOperator(hasOperatorName("!="), + hasEitherOperand(strCompare), + hasEitherOperand(integerLiteral(equals(0)))))) + .bind("match"), + this); +} + +void StringCompareCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *Matched = Result.Nodes.getNodeAs("match")) + diag(Matched->getLocStart(), + "do not use compare to test equality of strings; " + "use the string equality operator instead"); +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -75,6 +75,11 @@ - `misc-pointer-and-integral-operation` check was removed. +- New `misc-string-compare + `_ check + + Warns about using ``compare`` to test for string equality or ineqaulity. + - New `misc-use-after-move `_ check Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -80,6 +80,7 @@ misc-sizeof-container misc-sizeof-expression misc-static-assert + misc-string-compare misc-string-constructor misc-string-integer-assignment misc-string-literal-with-embedded-nul Index: docs/clang-tidy/checks/misc-string-compare.rst =================================================================== --- docs/clang-tidy/checks/misc-string-compare.rst +++ docs/clang-tidy/checks/misc-string-compare.rst @@ -0,0 +1,35 @@ +.. title:: clang-tidy - misc-string-compare + +misc-string-compare +=================== + +Finds string comparisons using the compare method. + +A common mistake is to use the string's ``compare`` method instead of using the +equality or inequality operators. The compare method is intended for sorting +functions and thus returns ``-1``, ``0`` or ``1`` depending on the lexicographical +relationship between the strings compared. If an equality or inequality check +can suffice, that is recommended. + +Examples: + +.. code-block:: c++ + + std::string str1{"a"}; + std::string str2{"b"}; + + if(str1.compare(str2)) {} // use str1 != str2 instead + + if(!str1.compare(str2)) {} // use str1 == str2 instead + + if(str1.compare(str2) == 0) {} // use str1 == str2 instead + + if(str1.compare(str2) != 0) {} // use str1 != str2 instead + + if(0 == str1.compare(str2)) {} // use str1 == str2 instead + + if(0 != str1.compare(str2)) {} // use str1 != str2 instead + +The above code examples shows the list of if-statements that this check will +give a warning for. All of them uses ``compare`` to check if equality or +inequality of two strings instead of using the correct operators. Index: test/clang-tidy/misc-string-compare.cpp =================================================================== --- test/clang-tidy/misc-string-compare.cpp +++ test/clang-tidy/misc-string-compare.cpp @@ -0,0 +1,49 @@ +// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11 + +namespace std { +template +class allocator {}; +template +class char_traits {}; +template , typename A = std::allocator > +struct basic_string { + basic_string(); + basic_string(const C*, unsigned int size); + int compare(const basic_string& str); +}; + bool operator==(const basic_string lhs, const basic_string rhs); + bool operator!=(const basic_string lhs, const basic_string rhs); +typedef basic_string string; +} + + + +void Test() { + std::string str1("a", 1); + std::string str2("b", 1); + + if(str1.compare(str2)) {} + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare] + if(!str1.compare(str2)) {} + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare] + if(str1.compare(str2) == 0) {} + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare] + if(str1.compare(str2) != 0) {} + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare] + if(0 == str1.compare(str2)) {} + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare] + if(0 != str1.compare(str2)) {} + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare] + +} + +void Valid() { + std::string str1("a", 1); + std::string str2("b", 1); + if(str1 == str2) {} + if(str1 != str2) {} + if(str1.compare(str2) == 1) {} + if(str1.compare(str2) == str1.compare(str2)) {} + if(0 == 0) {} + if(1 == str1.compare(str2)) {} +}