ztamas created this revision. Herald added subscribers: cfe-commits, xazax.hun. Herald added a project: clang.
Added WarnOnlyIfThisHasSuspiciousField option to allow to catch any copy assignment operator independently from the container class's fields. Added the cert alias using this option. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D62192 Files: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst clang-tools-extra/docs/clang-tidy/checks/cert-oop54-cpp.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment-warn-only-if-this-has-suspicious-field.cpp clang-tools-extra/test/clang-tidy/cert-oop54-cpp.cpp
Index: clang-tools-extra/test/clang-tidy/cert-oop54-cpp.cpp =================================================================== --- /dev/null +++ clang-tools-extra/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; +}; Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment-warn-only-if-this-has-suspicious-field.cpp =================================================================== --- /dev/null +++ clang-tools-extra/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: clang-tools-extra/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/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-oop11-cpp> + cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) <cert-oop54-cpp> cppcoreguidelines-avoid-c-arrays (redirects to modernize-avoid-c-arrays) <cppcoreguidelines-avoid-c-arrays> cppcoreguidelines-avoid-goto cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) <cppcoreguidelines-avoid-magic-numbers> Index: clang-tools-extra/docs/clang-tidy/checks/cert-oop54-cpp.rst =================================================================== --- /dev/null +++ clang-tools-extra/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 <bugprone-unhandled-self-assignment.html>`_ +for more information. Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst +++ clang-tools-extra/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 +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 field (pointer or C array). This option is set to `1` by default. Index: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp +++ clang-tools-extra/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<performance::MoveConstructorInitCheck>( "cert-oop11-cpp"); + CheckFactories.registerCheck<bugprone::UnhandledSelfAssignmentCheck>( + "cert-oop54-cpp"); // ERR CheckFactories.registerCheck<misc::ThrowByValueCatchByReferenceCheck>( "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-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h =================================================================== --- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h +++ clang-tools-extra/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-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp =================================================================== --- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp +++ clang-tools-extra/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(
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits