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,89 @@ +//===--- 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 "../utils/FixItHintUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Tooling/FixIt.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, + anyOf( + callExpr(has(implicitCastExpr(has(declRefExpr())))).bind("str2"), + unaryOperator( + hasOperatorName("*"), + hasUnaryOperand(implicitCastExpr(has(declRefExpr())))) + .bind("str2"), + declRefExpr().bind("str2"), stringLiteral().bind("str2"))), + argumentCountIs(1), + callee(memberExpr(has(implicitCastExpr(anyOf( + has(callExpr(has(implicitCastExpr(has(declRefExpr())))).bind("str1")), + has(declRefExpr().bind("str1")), + has(implicitCastExpr(has(declRefExpr().bind("str1"))) + .bind("str1pointer")))))))); + + // First and second case: cast str.compare(str) to boolean. + Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(booleanType()), + has(strCompare)) + .bind("match1"), + this); + + // Third and fourth case: str.compare(str) == 0 and str.compare(str) != 0. + Finder->addMatcher( + binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")), + hasEitherOperand(strCompare.bind("compare")), + hasEitherOperand(integerLiteral(equals(0)).bind("zero"))) + .bind("match2"), + this); +} + +void StringCompareCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *Matched = Result.Nodes.getNodeAs("match1")) + diag(Matched->getLocStart(), + "do not use 'compare' to test equality of strings; " + "use the string equality operator instead"); + + if (const auto *Matched = Result.Nodes.getNodeAs("match2")) { + const ASTContext &Ctx = *Result.Context; + const auto *zero = Result.Nodes.getNodeAs("zero"); + const auto *str1 = Result.Nodes.getNodeAs("str1"); + const auto *str2 = Result.Nodes.getNodeAs("str2"); + const auto *compare = Result.Nodes.getNodeAs("compare"); + + auto Diag = diag(Matched->getLocStart(), + "do not use 'compare' to test equality of strings; " + "use the string equality operator instead"); + + if (const auto *str1pointer = Result.Nodes.getNodeAs("str1pointer")) + Diag << FixItHint::CreateInsertion(str1pointer->getLocStart(), "*"); + + Diag << tooling::fixit::createReplacement(*zero, *str2, Ctx) + << tooling::fixit::createReplacement(*compare, *str1, Ctx); + } + + // FIXME: Add fixit to fix the code for case one and two (match1). +} + +} // 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,54 @@ +.. 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 a negative number, a positive number or +zero depending on the lexicographical relationship between the strings compared. +If an equality or inequality check can suffice, that is recommended. This is +recommended to avoid the risk of incorrect interpretation of the return value +and to simplify the code. The string equality and inequality operators can +also be faster than the ``compare`` method due to early termination. + +Examples: + +.. code-block:: c++ + + std::string str1{"a"}; + std::string str2{"b"}; + + // use str1 != str2 instead. + 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 == "foo" instead. + if (str1.compare("foo") == 0) { + } + +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,111 @@ +// 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> +class basic_string { +public: + basic_string(); + basic_string(const C *, unsigned int size); + int compare(const basic_string &str) const; + int compare(const C *) const; + int compare(int, int, const basic_string &str) const; + bool empty(); +}; +bool operator==(const basic_string &lhs, const basic_string &rhs); +bool operator!=(const basic_string &lhs, const basic_string &rhs); +bool operator==(const basic_string &lhs, const char *&rhs); +typedef basic_string string; +} + +void func(bool b); + +std::string comp() { + std::string str("a", 1); + return str; +} + +void Test() { + std::string str1("a", 1); + std::string str2("b", 1); + + if (str1.compare(str2)) { + } + // CHECK-MESSAGES: [[@LINE-2]]:7: 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-2]]:8: 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-2]]:7: warning: do not use 'compare' to test equality of strings; + // CHECK-FIXES: if (str1 == str2) { + if (str1.compare(str2) != 0) { + } + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; + // CHECK-FIXES: if (str1 != str2) { + if (str1.compare("foo") == 0) { + } + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; + // CHECK-FIXES: if (str1 == "foo") { + if (0 == str1.compare(str2)) { + } + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; + // CHECK-FIXES: if (str2 == str1) { + if (0 != str1.compare(str2)) { + } + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; + // CHECK-FIXES: if (str2 != str1) { + func(str1.compare(str2)); + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use 'compare' to test equality of strings; + if (str2.empty() || str1.compare(str2) != 0) { + } + // CHECK-MESSAGES: [[@LINE-2]]:23: warning: do not use 'compare' to test equality of strings; + // CHECK-FIXES: if (str2.empty() || str1 != str2) { + std::string *str3 = &str1; + if (str3->compare(str2)) { + } + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; + if (str3->compare(str2) == 0) { + } + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; + // CHECK-FIXES: if (*str3 == str2) { + if (str2.compare(*str3) == 0) { + } + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; + // CHECK-FIXES: if (str2 == *str3) { + if (comp().compare(str1) == 0) { + } + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; + // CHECK-FIXES: if (comp() == str1) { + if (str1.compare(comp()) == 0) { + } + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; + // CHECK-FIXES: if (str1 == comp()) { + if (str1.compare(comp())) { + } + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; +} + +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)) { + } + if (str1.compare(str2) > 0) { + } + if (str1.compare(1, 3, str2)) { + } +}