[clang-tidy] Add check 'misc-string-compare'.

I have a created a new check for clang tidy: misc-string-compare. This will check for incorrect usage of std::string::compare when used to check equality or inequality of string instead of the string equality or inequality operators.

Example:
```
  std::string str1, str2;
  if (str1.compare(str2)) {
  }
```

Reviewers: hokein, aaron.ballman, alexfh, malcolm.parsons

Subscribers: xazax.hun, Eugene.Zelenko, cfe-commits, malcolm.parsons, Prazek, mgorny, JDevlieghere

Differential Revision: https://reviews.llvm.org/D27210


git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@290747 91177308-0d34-0410-b5e6-96231b3b80d8

8 files changed, 300 insertions, 0 deletions

diff --git a/clang-tidy/misc/CMakeLists.txt b/clang-tidy/misc/CMakeLists.txt
index 4014658e..dd01f52b 100644
--- a/clang-tidy/misc/CMakeLists.txt
+++ b/clang-tidy/misc/CMakeLists.txt
@@ -28,6 +28,7 @@ add_clang_library(clangTidyMiscModule
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   StaticAssertCheck.cpp
+  StringCompareCheck.cpp
   StringConstructorCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
diff --git a/clang-tidy/misc/MiscTidyModule.cpp b/clang-tidy/misc/MiscTidyModule.cpp
index 0499e067..755e2c7a 100644
--- a/clang-tidy/misc/MiscTidyModule.cpp
+++ b/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"
@@ -106,6 +107,7 @@ public:
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "misc-sizeof-expression");
     CheckFactories.registerCheck<StaticAssertCheck>("misc-static-assert");
+    CheckFactories.registerCheck<StringCompareCheck>("misc-string-compare");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "misc-string-constructor");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
diff --git a/clang-tidy/misc/StringCompareCheck.cpp b/clang-tidy/misc/StringCompareCheck.cpp
new file mode 100644
index 00000000..b6a42953
--- /dev/null
+++ b/clang-tidy/misc/StringCompareCheck.cpp
@@ -0,0 +1,82 @@
+//===--- 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 {
+
+static const StringRef CompareMessage = "do not use 'compare' to test equality "
+                                        "of strings; use the string equality "
+                                        "operator instead";
+
+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, expr().bind("str2")), argumentCountIs(1),
+      callee(memberExpr().bind("str1")));
+
+  // 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<Stmt>("match1")) {
+    diag(Matched->getLocStart(), CompareMessage);
+    return;
+  }
+
+  if (const auto *Matched = Result.Nodes.getNodeAs<Stmt>("match2")) {
+    const ASTContext &Ctx = *Result.Context;
+
+    if (const auto *Zero = Result.Nodes.getNodeAs<Stmt>("zero")) {
+      const auto *Str1 = Result.Nodes.getNodeAs<MemberExpr>("str1");
+      const auto *Str2 = Result.Nodes.getNodeAs<Stmt>("str2");
+      const auto *Compare = Result.Nodes.getNodeAs<Stmt>("compare");
+
+      auto Diag = diag(Matched->getLocStart(), CompareMessage);
+
+      if (Str1->isArrow())
+        Diag << FixItHint::CreateInsertion(Str1->getLocStart(), "*");
+
+      Diag << tooling::fixit::createReplacement(*Zero, *Str2, Ctx)
+           << tooling::fixit::createReplacement(*Compare, *Str1->getBase(),
+                                                Ctx);
+    }
+  }
+
+  // FIXME: Add fixit to fix the code for case one and two (match1).
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tidy/misc/StringCompareCheck.h b/clang-tidy/misc/StringCompareCheck.h
new file mode 100644
index 00000000..2f3a7d65
--- /dev/null
+++ b/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 LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_COMPARE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_COMPARE_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.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 // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_COMPARE_H
diff --git a/docs/ReleaseNotes.rst b/docs/ReleaseNotes.rst
index bf481a5f..813e496e 100644
--- a/docs/ReleaseNotes.rst
+++ b/docs/ReleaseNotes.rst
@@ -80,6 +80,11 @@ Improvements to clang-tidy
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare.html>`_ check
+
+  Warns about using ``compare`` to test for string equality or inequality.
+
 - New `misc-use-after-move
   <http://clang.llvm.org/extra/clang-tidy/checks/misc-use-after-move.html>`_ check
 
diff --git a/docs/clang-tidy/checks/list.rst b/docs/clang-tidy/checks/list.rst
index 64f2cd18..e16d3323 100644
--- a/docs/clang-tidy/checks/list.rst
+++ b/docs/clang-tidy/checks/list.rst
@@ -81,6 +81,7 @@ Clang-Tidy Checks
    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
diff --git a/docs/clang-tidy/checks/misc-string-compare.rst b/docs/clang-tidy/checks/misc-string-compare.rst
new file mode 100644
index 00000000..78a5d4f6
--- /dev/null
+++ b/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.
diff --git a/test/clang-tidy/misc-string-compare.cpp b/test/clang-tidy/misc-string-compare.cpp
new file mode 100644
index 00000000..9b6ef1ce
--- /dev/null
+++ b/test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,119 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template <typename T>
+class allocator {};
+template <typename T>
+class char_traits {};
+template <typename C, typename T = std::char_traits<C>, typename A = std::allocator<C>>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string<char> &str) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string<char> &str) const;
+  bool empty();
+};
+bool operator==(const basic_string<char> &lhs, const basic_string<char> &rhs);
+bool operator!=(const basic_string<char> &lhs, const basic_string<char> &rhs);
+bool operator==(const basic_string<char> &lhs, const char *&rhs);
+typedef basic_string<char> 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) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(1, 3, str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(str2) < 0) {
+  }
+  if (str1.compare(str2) == 2) {
+  }
+  if (str1.compare(str2) == -3) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == -1) {
+  }
+}