diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp index 27fda49a4e500..97c352ba95218 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 @@ class CppCoreGuidelinesModule : public ClangTidyModule { "cppcoreguidelines-pro-type-union-access"); CheckFactories.registerCheck( "cppcoreguidelines-pro-type-vararg"); - CheckFactories.registerCheck( + CheckFactories.registerCheck( "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 3aee73ecb8044..c7f3c14f62488 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 9aecdc1c4472a..cae3f0dc13809 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 @@ class MiscModule : public ClangTidyModule { CheckFactories.registerCheck("misc-argument-comment"); CheckFactories.registerCheck( "misc-assert-side-effect"); - CheckFactories.registerCheck( - "misc-assign-operator-signature"); + CheckFactories.registerCheck( + "misc-unconventional-assign-operator"); CheckFactories.registerCheck( "misc-bool-pointer-implicit-conversion"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/misc/AssignOperatorSignatureCheck.cpp b/clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp similarity index 58% rename from clang-tools-extra/clang-tidy/misc/AssignOperatorSignatureCheck.cpp rename to clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp index 1112431192b10..53cc0825b7a86 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("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")) { + 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(Message[0])) - diag(Method->getLocStart(), Message[1]) - << Name << (Method->isConst() ? "const" : "virtual"); + const auto *Method = Result.Nodes.getNodeAs("method"); + for (const auto &Message : Messages) { + if (Result.Nodes.getNodeAs(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 similarity index 62% rename from clang-tools-extra/clang-tidy/misc/AssignOperatorSignatureCheck.h rename to clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.h index a2530f9314cfd..5545de0772953 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; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index e4a88b21de9d8..5a246115b8cf9 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -236,6 +236,12 @@ identified. The improvements since the 3.8 release include: Finds static function and variable definitions in anonymous namespace. +- New `misc-unconventional-assign-operator + `_ check replacing old `misc-assign-operator-signature` check + + Does not only checks for correct signature but also for correct ``return`` + statements (returning ``*this``) + Fixed bugs: - Crash when running on compile database with relative source files paths. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index d5c680c653ce7..7459734afda84 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -52,7 +52,7 @@ Clang-Tidy Checks llvm-twine-local misc-argument-comment misc-assert-side-effect - misc-assign-operator-signature + misc-unconventional-assign-operator misc-bool-pointer-implicit-conversion misc-dangling-handle misc-definitions-in-headers diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-assign-operator-signature.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-assign-operator-signature.rst deleted file mode 100644 index dc34e1181a1fb..0000000000000 --- a/clang-tools-extra/docs/clang-tidy/checks/misc-assign-operator-signature.rst +++ /dev/null @@ -1,12 +0,0 @@ -.. title:: clang-tidy - misc-assign-operator-signature - -misc-assign-operator-signature -============================== - - -Finds declarations of assign operators with the wrong return and/or argument -types. - - * The return type must be ``Class&``. - * Works with move-assign and assign by value. - * Private and deleted operators are ignored. diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-unconventional-assign-operator.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-unconventional-assign-operator.rst new file mode 100644 index 0000000000000..e12241b071bb0 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc-unconventional-assign-operator.rst @@ -0,0 +1,13 @@ +.. title:: clang-tidy - misc-unconventional-assign-operator + +misc-unconventional-assign-operator +==================== + + +Finds declarations of assign 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. + * The operator must always return ``*this``. diff --git a/clang-tools-extra/test/clang-tidy/misc-assign-operator-signature.cpp b/clang-tools-extra/test/clang-tidy/misc-unconventional-assign-operator.cpp similarity index 64% rename from clang-tools-extra/test/clang-tidy/misc-assign-operator-signature.cpp rename to clang-tools-extra/test/clang-tidy/misc-unconventional-assign-operator.cpp index 8609fb916846a..3041f593ec134 100644 --- a/clang-tools-extra/test/clang-tidy/misc-assign-operator-signature.cpp +++ b/clang-tools-extra/test/clang-tidy/misc-unconventional-assign-operator.cpp @@ -1,4 +1,6 @@ -// RUN: %check_clang_tidy %s misc-assign-operator-signature %t +// RUN: %check_clang_tidy %s misc-unconventional-assign-operator %t -- -- -std=c++11 -isystem %S/Inputs/Headers + +#include struct Good { Good& operator=(const Good&); @@ -13,18 +15,19 @@ struct AlsoGood { 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&&); +struct BadReturnType { + void operator=(const BadReturnType&); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'BadReturnType&' [misc-unconventional-assign-operator] + const BadReturnType& operator=(BadReturnType&&); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad void operator=(int); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad }; -struct BadReturn2 { - BadReturn2&& operator=(const BadReturn2&); + +struct BadReturnType2 { + BadReturnType2&& operator=(const BadReturnType2&); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad - int operator=(BadReturn2&&); + int operator=(BadReturnType2&&); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad }; @@ -56,3 +59,21 @@ struct Virtual { virtual Virtual& operator=(const Virtual &); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should not be marked 'virtual' }; + +class BadReturnStatement { + int n; + +public: + BadReturnStatement& operator=(BadReturnStatement&& rhs) { + n = std::move(rhs.n); + return rhs; +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: operator=() should always return '*this' + } + + // Do not check if return type is different from '&BadReturnStatement' + int operator=(int i) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad + n = i; + return n; + } +};