[clang-tidy] New: checker misc-unconventional-assign-operator replacing misc-assign-operator-signature

Summary: Finds return statements in assign operator bodies where the return value is different from '*this'. Only assignment operators with correct return value Class& are checked.

Reviewers: aaron.ballman, alexfh, sbenza

Subscribers: o.gyorgy, baloghadamsoftware, LegalizeAdulthood, aaron.ballman, Eugene.Zelenko, xazax.hun, cfe-commits

Differential Revision: http://reviews.llvm.org/D18265

llvm-svn: 268492

5 files changed, 42 insertions, 28 deletions

diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
index 27fda49a4e5..97c352ba952 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -10,7 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
-#include "../misc/AssignOperatorSignatureCheck.h"
+#include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsConstantArrayIndexCheck.h"
@@ -53,7 +53,7 @@ public:
         "cppcoreguidelines-pro-type-union-access");
     CheckFactories.registerCheck<ProTypeVarargCheck>(
         "cppcoreguidelines-pro-type-vararg");
-    CheckFactories.registerCheck<misc::AssignOperatorSignatureCheck>(
+    CheckFactories.registerCheck<misc::UnconventionalAssignOperatorCheck>(
         "cppcoreguidelines-c-copy-assignment-signature");
   }
 };
diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
index 3aee73ecb80..c7f3c14f624 100644
--- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
@@ -3,7 +3,7 @@ set(LLVM_LINK_COMPONENTS support)
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
-  AssignOperatorSignatureCheck.cpp
+  UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DanglingHandleCheck.cpp
   DefinitionsInHeadersCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
index 9aecdc1c447..cae3f0dc138 100644
--- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -12,7 +12,7 @@
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
-#include "AssignOperatorSignatureCheck.h"
+#include "UnconventionalAssignOperatorCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "DanglingHandleCheck.h"
 #include "DefinitionsInHeadersCheck.h"
@@ -61,8 +61,8 @@ public:
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "misc-assert-side-effect");
-    CheckFactories.registerCheck<AssignOperatorSignatureCheck>(
-        "misc-assign-operator-signature");
+    CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
+        "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "misc-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<DanglingHandleCheck>(
diff --git a/clang-tools-extra/clang-tidy/misc/AssignOperatorSignatureCheck.cpp b/clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
index 1112431192b..53cc0825b7a 100644
--- a/clang-tools-extra/clang-tidy/misc/AssignOperatorSignatureCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
@@ -1,4 +1,4 @@
-//===--- AssignOperatorSignatureCheck.cpp - clang-tidy ----------*- C++ -*-===//
+//===--- UnconventionalUnconventionalAssignOperatorCheck.cpp - clang-tidy -----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -7,7 +7,7 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "AssignOperatorSignatureCheck.h"
+#include "UnconventionalAssignOperatorCheck.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 
@@ -17,8 +17,7 @@ namespace clang {
 namespace tidy {
 namespace misc {
 
-void AssignOperatorSignatureCheck::registerMatchers(
-    ast_matchers::MatchFinder *Finder) {
+void UnconventionalAssignOperatorCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
   // Only register the matchers for C++; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
   if (!getLangOpts().CPlusPlus)
@@ -56,22 +55,32 @@ void AssignOperatorSignatureCheck::registerMatchers(
   Finder->addMatcher(
       cxxMethodDecl(IsSelfAssign, anyOf(isConst(), isVirtual())).bind("cv"),
       this);
-}
 
-void AssignOperatorSignatureCheck::check(
-    const MatchFinder::MatchResult &Result) {
-  const auto *Method = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
-  std::string Name = Method->getParent()->getName();
+  const auto IsBadReturnStatement = returnStmt(unless(has(
+      unaryOperator(hasOperatorName("*"), hasUnaryOperand(cxxThisExpr())))));
+  const auto IsGoodAssign = cxxMethodDecl(IsAssign, HasGoodReturnType);
+
+  Finder->addMatcher(returnStmt(IsBadReturnStatement, forFunction(IsGoodAssign))
+                         .bind("returnStmt"),
+                     this);
+}
 
-  static const char *const Messages[][2] = {
-      {"ReturnType", "operator=() should return '%0&'"},
-      {"ArgumentType", "operator=() should take '%0 const&', '%0&&' or '%0'"},
-      {"cv", "operator=() should not be marked '%1'"}};
+void UnconventionalAssignOperatorCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *RetStmt = Result.Nodes.getNodeAs<ReturnStmt>("returnStmt")) {
+    diag(RetStmt->getLocStart(), "operator=() should always return '*this'");
+  } else {
+    static const char *const Messages[][2] = {
+        {"ReturnType", "operator=() should return '%0&'"},
+        {"ArgumentType", "operator=() should take '%0 const&', '%0&&' or '%0'"},
+        {"cv", "operator=() should not be marked '%1'"}};
 
-  for (const auto &Message : Messages) {
-    if (Result.Nodes.getNodeAs<Decl>(Message[0]))
-      diag(Method->getLocStart(), Message[1])
-          << Name << (Method->isConst() ? "const" : "virtual");
+    const auto *Method = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
+    for (const auto &Message : Messages) {
+      if (Result.Nodes.getNodeAs<Decl>(Message[0]))
+        diag(Method->getLocStart(), Message[1])
+            << Method->getParent()->getName()
+            << (Method->isConst() ? "const" : "virtual");
+    }
   }
 }
 
diff --git a/clang-tools-extra/clang-tidy/misc/AssignOperatorSignatureCheck.h b/clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.h
index a2530f9314c..5545de07729 100644
--- a/clang-tools-extra/clang-tidy/misc/AssignOperatorSignatureCheck.h
+++ b/clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.h
@@ -1,4 +1,4 @@
-//===--- AssignOperatorSignatureCheck.h - clang-tidy ------------*- C++ -*-===//
+//===--- UnconventionalUnconventionalAssignOperatorCheck.h - clang-tidy -------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -16,15 +16,20 @@ namespace clang {
 namespace tidy {
 namespace misc {
 
-/// Finds declarations of assign operators with the wrong return and/or argument
-/// types.
+/// Finds declarations of assignment operators with the wrong return and/or
+/// argument types and definitions with good return type but wrong return
+/// statements.
 ///
 ///   * The return type must be `Class&`.
 ///   * Works with move-assign and assign by value.
 ///   * Private and deleted operators are ignored.
-class AssignOperatorSignatureCheck : public ClangTidyCheck {
+///   * The operator must always return ``*this``.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-unconventional-assign-operator.html
+class UnconventionalAssignOperatorCheck : public ClangTidyCheck {
 public:
-  AssignOperatorSignatureCheck(StringRef Name, ClangTidyContext *Context)
+  UnconventionalAssignOperatorCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;