Verify assign operator signatures.

[sbenza]

Summary

Warn when the return type of assign operators is not Class&.

[sbenza]

Ignore private operators too.

[alexfh]

Sorry for the long delay.

This looks mostly fine, but I wonder if it makes sense to check for other possible mistakes, e.g.:

T& operator=(T&);
const T& operator=(const T&) const;
virtual T& operator=(const T&);

Also, it seems like this issue is not specific to the Google C++ Style, so it could be put into misc/ or it may even be reasonable to have a compiler warning for this.

clang-tidy/google/AssignOperatorSignatureCheck.cpp

23 ↗

(On Diff #17301)

I guess you missed "unless(isImplicit())" (and a test for it). And maybe also unless(isInTemplateInstantiation()) (especially, if you would want to provide a fixit for the "const T& operator=(const T&)" case).

clang-tidy/google/AssignOperatorSignatureCheck.h

25 ↗

(On Diff #17301)

Unfortunately, this needs to be done the old way. LLVM supports MSVS 2013 which doesn't support this form of constructor delegation.

[sbenza]

Added more checks.
Also fixed assigning by-value and assigning from a different type. Those are ok.

clang-tidy/google/AssignOperatorSignatureCheck.cpp

23 ↗

(On Diff #17301)

I could add unless(isImplicit()), but the implicit one will have the right signature, so it shouldn't match anyway.

clang-tidy/google/AssignOperatorSignatureCheck.h

25 ↗

(On Diff #17301)

Done.

[sbenza]

Add more checks.

[alexfh]

What about virtual T& operator=(const T&);? It looks like a bad pattern, IMHO.

Also, it looks like the check is not specific to Google and it's not exactly about readability. Maybe move it to misc/?

clang-tidy/google/AssignOperatorSignatureCheck.cpp

23 ↗

(On Diff #17301)

Reasonable. Maybe a comment would not be useless.

[sbenza]

Add isImplicit()

[sbenza]

wrt virtual, I don't know. Maybe there is a good reason to make it virtual? Doesn't seem like a common mistake anyway.
wrt category for the check, I agree. I'll move it once we have no more comments on the code.

clang-tidy/google/AssignOperatorSignatureCheck.cpp

23 ↗

(On Diff #17301)

I just added the call. It is correct and self documenting.

[alexfh]

Looks good! (After moving to misc/.)

[sbenza]

Move to misc-

[alexfh]

test/clang-tidy/misc-assign-operator.cpp

1 ↗

(On Diff #19583)

Please name the file "misc-assign-operator-signature.cpp".

[sbenza]

Rename test file

[alexfh]

Looks good!

clang-tools-extra/trunk/clang-tidy/misc/AssignOperatorSignatureCheck.h

+//===--- AssignOperatorSignatureCheck.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_ASSIGN_OPERATOR_SIGNATURE_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSIGN_OPERATOR_SIGNATURE_CHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Finds declarations of assign operators with the wrong return and/or
+///   argument types.
+///
+/// The return type must be \c Class&.
+/// Works with move-assign and assign by value.
+/// Private and deleted operators are ignored.
+class AssignOperatorSignatureCheck : public ClangTidyCheck {
+public:
+  AssignOperatorSignatureCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSIGN_OPERATOR_SIGNATURE_CHECK_H

clang-tools-extra/trunk/clang-tidy/misc/AssignOperatorSignatureCheck.cpp

+//===--- AssignOperatorSignatureCheck.cpp - clang-tidy ----------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "AssignOperatorSignatureCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void AssignOperatorSignatureCheck::registerMatchers(
+    ast_matchers::MatchFinder *Finder) {
+  const auto HasGoodReturnType = methodDecl(returns(lValueReferenceType(pointee(
+      unless(isConstQualified()), hasDeclaration(equalsBoundNode("class"))))));
+
+  const auto IsSelf = qualType(
+      anyOf(hasDeclaration(equalsBoundNode("class")),
+            referenceType(pointee(hasDeclaration(equalsBoundNode("class"))))));
+  const auto IsSelfAssign =
+      methodDecl(unless(anyOf(isDeleted(), isPrivate(), isImplicit())),
+                 hasName("operator="), ofClass(recordDecl().bind("class")),
+                 hasParameter(0, parmVarDecl(hasType(IsSelf)))).bind("method");
+
+  Finder->addMatcher(
+      methodDecl(IsSelfAssign, unless(HasGoodReturnType)).bind("ReturnType"),
+      this);
+
+  const auto BadSelf = referenceType(
+      anyOf(lValueReferenceType(pointee(unless(isConstQualified()))),
+            rValueReferenceType(pointee(isConstQualified()))));
+
+  Finder->addMatcher(
+      methodDecl(IsSelfAssign, hasParameter(0, parmVarDecl(hasType(BadSelf))))
+          .bind("ArgumentType"),
+      this);
+
+  Finder->addMatcher(methodDecl(IsSelfAssign, isConst()).bind("Const"), this);
+}
+
+
+void AssignOperatorSignatureCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto* Method = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
+  std::string Name = Method->getParent()->getName();
+
+  static const char *Messages[][2] = {
+      {"ReturnType", "operator=() should return '%0&'"},
+      {"ArgumentType", "operator=() should take '%0 const&', '%0&&' or '%0'"},
+      {"Const", "operator=() should not be marked 'const'"},
+  };
+
+  for (const auto& Message : Messages) {
+    if (Result.Nodes.getNodeAs<Decl>(Message[0]))
+      diag(Method->getLocStart(), Message[1]) << Name;
+  }
+}
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
+  AssignOperatorSignatureCheck.cpp
   BoolPointerImplicitConversion.cpp
   InefficientAlgorithmCheck.cpp
   MiscTidyModule.cpp
   SwappedArgumentsCheck.cpp
   UndelegatedConstructor.cpp
   UnusedRAII.cpp
   UniqueptrResetRelease.cpp
   UseOverride.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
+#include "AssignOperatorSignatureCheck.h"
 #include "BoolPointerImplicitConversion.h"
 #include "InefficientAlgorithmCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetRelease.h"
 #include "UnusedRAII.h"
 #include "UseOverride.h"
 
 namespace clang {
 namespace tidy {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
+    CheckFactories.registerCheck<AssignOperatorSignatureCheck>(
+        "misc-assign-operator-signature");
     CheckFactories.registerCheck<BoolPointerImplicitConversion>(
         "misc-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<InefficientAlgorithmCheck>(
         "misc-inefficient-algorithm");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "misc-undelegated-constructor");

clang-tools-extra/trunk/test/clang-tidy/misc-assign-operator-signature.cpp

+// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-assign-operator-signature %t
+// REQUIRES: shell
+
+struct Good {
+  Good& operator=(const Good&);
+  Good& operator=(Good&&);
+
+  // Assign from other types is fine too.
+  Good& operator=(int);
+};
+
+struct AlsoGood {
+  // By value is also fine.
+  AlsoGood& operator=(AlsoGood);
+};
+
+struct BadReturn {
+  void operator=(const BadReturn&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'BadReturn&' [misc-assign-operator-signature]
+  const BadReturn& operator=(BadReturn&&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
+};
+struct BadReturn2 {
+  BadReturn2&& operator=(const BadReturn2&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
+  int operator=(BadReturn2&&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
+};
+
+struct BadArgument {
+  BadArgument& operator=(BadArgument&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadArgument const&', 'BadArgument&&' or 'BadArgument'
+  BadArgument& operator=(const BadArgument&&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadAr
+};
+
+struct BadModifier {
+  BadModifier& operator=(const BadModifier&) const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should not be marked 'const'
+};
+
+struct Deleted {
+  // We don't check the return value of deleted operators.
+  void operator=(const Deleted&) = delete;
+  void operator=(Deleted&&) = delete;
+};
+
+class Private {
+  // We don't check the return value of private operators.
+  // Pre-C++11 way of disabling assignment.
+  void operator=(const Private &);
+};