Index: clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h =================================================================== --- clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h +++ clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h @@ -23,10 +23,14 @@ /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unhandled-self-assignment.html class UnhandledSelfAssignmentCheck : public ClangTidyCheck { public: - UnhandledSelfAssignmentCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + UnhandledSelfAssignmentCheck(StringRef Name, ClangTidyContext *Context); + + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const bool WarnOnlyIfThisHasSuspiciousField; }; } // namespace bugprone Index: clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp =================================================================== --- clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp +++ clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp @@ -16,6 +16,18 @@ namespace tidy { namespace bugprone { +UnhandledSelfAssignmentCheck::UnhandledSelfAssignmentCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + WarnOnlyIfThisHasSuspiciousField( + Options.get("WarnOnlyIfThisHasSuspiciousField", true)) {} + +void UnhandledSelfAssignmentCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "WarnOnlyIfThisHasSuspiciousField", + WarnOnlyIfThisHasSuspiciousField); +} + void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) { if (!getLangOpts().CPlusPlus) return; @@ -61,29 +73,32 @@ cxxMethodDecl(unless(hasDescendant(cxxMemberCallExpr(callee(cxxMethodDecl( hasName("operator="), ofClass(equalsBoundNode("class")))))))); - // Matcher for standard smart pointers. - const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType( - recordType(hasDeclaration(classTemplateSpecializationDecl( - hasAnyName("::std::shared_ptr", "::std::unique_ptr", - "::std::weak_ptr", "::std::auto_ptr"), - templateArgumentCountIs(1)))))); - - // We will warn only if the class has a pointer or a C array field which - // probably causes a problem during self-assignment (e.g. first resetting the - // pointer member, then trying to access the object pointed by the pointer, or - // memcpy overlapping arrays). - const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl( - has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType), - hasType(arrayType()))))))); - - Finder->addMatcher( - cxxMethodDecl(ofClass(cxxRecordDecl().bind("class")), - isCopyAssignmentOperator(), IsUserDefined, - HasReferenceParam, HasNoSelfCheck, - unless(HasNonTemplateSelfCopy), unless(HasTemplateSelfCopy), - HasNoNestedSelfAssign, ThisHasSuspiciousField) - .bind("copyAssignmentOperator"), - this); + DeclarationMatcher AdditionalMatcher = cxxMethodDecl(); + if (WarnOnlyIfThisHasSuspiciousField) { + // Matcher for standard smart pointers. + const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(classTemplateSpecializationDecl( + hasAnyName("::std::shared_ptr", "::std::unique_ptr", + "::std::weak_ptr", "::std::auto_ptr"), + templateArgumentCountIs(1)))))); + + // We will warn only if the class has a pointer or a C array field which + // probably causes a problem during self-assignment (e.g. first resetting + // the pointer member, then trying to access the object pointed by the + // pointer, or memcpy overlapping arrays). + AdditionalMatcher = cxxMethodDecl(ofClass(cxxRecordDecl( + has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType), + hasType(arrayType()))))))); + } + + Finder->addMatcher(cxxMethodDecl(ofClass(cxxRecordDecl().bind("class")), + isCopyAssignmentOperator(), IsUserDefined, + HasReferenceParam, HasNoSelfCheck, + unless(HasNonTemplateSelfCopy), + unless(HasTemplateSelfCopy), + HasNoNestedSelfAssign, AdditionalMatcher) + .bind("copyAssignmentOperator"), + this); } void UnhandledSelfAssignmentCheck::check( Index: clang-tidy/cert/CERTTidyModule.cpp =================================================================== --- clang-tidy/cert/CERTTidyModule.cpp +++ clang-tidy/cert/CERTTidyModule.cpp @@ -9,6 +9,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "../bugprone/UnhandledSelfAssignmentCheck.h" #include "../google/UnnamedNamespaceInHeaderCheck.h" #include "../misc/NewDeleteOverloadsCheck.h" #include "../misc/NonCopyableObjects.h" @@ -49,6 +50,8 @@ // OOP CheckFactories.registerCheck( "cert-oop11-cpp"); + CheckFactories.registerCheck( + "cert-oop54-cpp"); // ERR CheckFactories.registerCheck( "cert-err09-cpp"); @@ -85,6 +88,7 @@ ClangTidyOptions Options; ClangTidyOptions::OptionMap &Opts = Options.CheckOptions; Opts["cert-dcl16-c.NewSuffixes"] = "L;LL;LU;LLU"; + Opts["cert-oop54-cpp.WarnOnlyIfThisHasSuspiciousField"] = "0"; return Options; } }; Index: clang-tidy/cert/CMakeLists.txt =================================================================== --- clang-tidy/cert/CMakeLists.txt +++ clang-tidy/cert/CMakeLists.txt @@ -20,6 +20,7 @@ clangBasic clangLex clangTidy + clangTidyBugproneModule clangTidyGoogleModule clangTidyMiscModule clangTidyPerformanceModule Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -134,6 +134,11 @@ subclasses of ``NSObject`` and recommends calling a superclass initializer instead. +- New alias :doc:`cert-oop54-cpp + ` to + :doc:`bugprone-unhandled-self-assignment + ` was added. + - New alias :doc:`cppcoreguidelines-explicit-virtual-functions ` to :doc:`modernize-use-override Index: docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst =================================================================== --- docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst +++ docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst @@ -3,11 +3,14 @@ bugprone-unhandled-self-assignment ================================== +`cert-oop54-cpp` redirects here as an alias for this check. For the CERT alias, +the `WarnOnlyIfThisHasSuspiciousField` option is set to `0`. + Finds user-defined copy assignment operators which do not protect the code against self-assignment either by checking self-assignment explicitly or using the copy-and-swap or the copy-and-move method. -This check now searches only those classes which have any pointer or C array field +By default, this check searches only those classes which have any pointer or C array field to avoid false positives. In case of a pointer or a C array, it's likely that self-copy assignment breaks the object if the copy assignment operator was not written with care. @@ -114,3 +117,8 @@ return *this; } }; + +.. option:: WarnOnlyIfThisHasSuspiciousField + + When non-zero, the check will warn only if the container class of the copy assignment operator + has any suspicious fields (pointer or C array). This option is set to `1` by default. Index: docs/clang-tidy/checks/cert-oop54-cpp.rst =================================================================== --- docs/clang-tidy/checks/cert-oop54-cpp.rst +++ docs/clang-tidy/checks/cert-oop54-cpp.rst @@ -0,0 +1,10 @@ +.. title:: clang-tidy - cert-oop54-cpp +.. meta:: + :http-equiv=refresh: 5;URL=bugprone-unhandled-self-assignment.html + +cert-oop54-cpp +============== + +The cert-oop54-cpp check is an alias, please see +`bugprone-unhandled-self-assignment `_ +for more information. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -98,6 +98,7 @@ cert-msc50-cpp cert-msc51-cpp cert-oop11-cpp (redirects to performance-move-constructor-init) + cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) cppcoreguidelines-avoid-c-arrays (redirects to modernize-avoid-c-arrays) cppcoreguidelines-avoid-goto cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) Index: test/clang-tidy/bugprone-unhandled-self-assignment-warn-only-if-this-has-suspicious-field.cpp =================================================================== --- test/clang-tidy/bugprone-unhandled-self-assignment-warn-only-if-this-has-suspicious-field.cpp +++ test/clang-tidy/bugprone-unhandled-self-assignment-warn-only-if-this-has-suspicious-field.cpp @@ -0,0 +1,41 @@ +// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t -- \ +// RUN: -config="{CheckOptions: \ +// RUN: [{key: bugprone-unhandled-self-assignment.WarnOnlyIfThisHasSuspiciousField, \ +// RUN: value: 0}]}" + +// Classes with pointer field are still caught. +class PtrField { +public: + PtrField &operator=(const PtrField &object) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment] + return *this; + } + +private: + int *p; +}; + +// With the option, check catches classes with trivial fields. +class TrivialFields { +public: + TrivialFields &operator=(const TrivialFields &object) { + // CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment] + return *this; + } + +private: + int m; + float f; + double d; + bool b; +}; + +// The check warns also when there is no field at all. +// In this case, user-defined copy assignment operator is useless anyway. +class ClassWithoutFields { +public: + ClassWithoutFields &operator=(const ClassWithoutFields &object) { + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment] + return *this; + } +}; Index: test/clang-tidy/cert-oop54-cpp.cpp =================================================================== --- test/clang-tidy/cert-oop54-cpp.cpp +++ test/clang-tidy/cert-oop54-cpp.cpp @@ -0,0 +1,16 @@ +// RUN: %check_clang_tidy %s cert-oop54-cpp %t + +// Test whether bugprone-unhandled-self-assignment.WarnOnlyIfThisHasSuspiciousField option is set correctly. +class TrivialFields { +public: + TrivialFields &operator=(const TrivialFields &object) { + // CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() does not handle self-assignment properly [cert-oop54-cpp] + return *this; + } + +private: + int m; + float f; + double d; + bool b; +};