gbencze updated this revision to Diff 228758. gbencze added a comment. Update documentation and release notes.
CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70052/new/ https://reviews.llvm.org/D70052 Files: clang-tools-extra/clang-tidy/misc/CMakeLists.txt clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp clang-tools-extra/clang-tidy/misc/MutatingCopyCheck.cpp clang-tools-extra/clang-tidy/misc/MutatingCopyCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/docs/clang-tidy/checks/misc-mutating-copy.rst clang-tools-extra/test/clang-tidy/misc-mutating-copy.cpp
Index: clang-tools-extra/test/clang-tidy/misc-mutating-copy.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/misc-mutating-copy.cpp @@ -0,0 +1,107 @@ +// RUN: %check_clang_tidy %s misc-mutating-copy %t -std=c++11-or-later + +// Example test cases from CERT rule +// https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP58-CPP.+Copy+operations+must+not+mutate+the+source+object +namespace test_mutating_noncompliant_example { +class A { + mutable int m; + +public: + A() : m(0) {} + explicit A(int m) : m(m) {} + + A(const A &other) : m(other.m) { + other.m = 0; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object + } + + A &operator=(const A &other) { + if (&other != this) { + m = other.m; + other.m = 0; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: mutating copied object + } + return *this; + } + + int get_m() const { return m; } +}; +} // namespace test_mutating_noncompliant_example + +namespace test_mutating_compliant_example { +class B { + int m; + +public: + B() : m(0) {} + explicit B(int m) : m(m) {} + + B(const B &other) : m(other.m) {} + B(B &&other) : m(other.m) { + other.m = 0; //no-warning: mutation allowed in move constructor + } + + B &operator=(const B &other) { + if (&other != this) { + m = other.m; + } + return *this; + } + + B &operator=(B &&other) { + m = other.m; + other.m = 0; //no-warning: mutation allowed in move assignment operator + return *this; + } + + int get_m() const { return m; } +}; +} // namespace test_mutating_compliant_example + +namespace test_mutating_pointer { +class C { + C *ptr; + int value; + + C(); + C(C &other) { + other = {}; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object + other.ptr = nullptr; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object + other.value = 0; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object + + // no-warning: mutating a pointee is allowed + other.ptr->value = 0; + *other.ptr = {}; + } +}; +} // namespace test_mutating_pointer + +namespace test_mutating_indirect_member { +struct S { + int x; +}; + +class D { + S s; + D(D &other) { + other.s = {}; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object + other.s.x = 0; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object + } +}; +} // namespace test_mutating_indirect_member + +namespace test_mutating_other_object { +class E { + E(); + E(E &other) { + E tmp; + // no-warning: mutating an object that is not the source is allowed + tmp = {}; + } +}; +} // namespace test_mutating_other_object Index: clang-tools-extra/docs/clang-tidy/checks/misc-mutating-copy.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/misc-mutating-copy.rst @@ -0,0 +1,11 @@ +.. title:: clang-tidy - misc-mutating-copy + +misc-mutating-copy +================== + +Finds assignments to the copied object and its direct or indirect members +in copy constructors and copy assignment operators. + +This check corresponds to the CERT C Coding Standard rule +`OOP58-CPP. Copy operations must not mutate the source object +<https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP58-CPP.+Copy+operations+must+not+mutate+the+source+object>`_. 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 @@ -283,6 +283,7 @@ llvm-twine-local misc-definitions-in-headers misc-misplaced-const + misc-mutating-copy misc-new-delete-overloads misc-non-copyable-objects misc-non-private-member-variables-in-classes Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -120,6 +120,12 @@ Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites them to use ``Register`` +- New :doc:`misc-mutating-copy + <clang-tidy/checks/misc-mutating-copy>` check. + + Finds assignments to the copied object and its direct or indirect members + in copy constructors and copy assignment operators. + - New :doc:`objc-missing-hash <clang-tidy/checks/objc-missing-hash>` check. Index: clang-tools-extra/clang-tidy/misc/MutatingCopyCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/misc/MutatingCopyCheck.h @@ -0,0 +1,35 @@ +//===--- MutatingCopyCheck.h - clang-tidy -----------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MUTATINGCOPYCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MUTATINGCOPYCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// Finds assignments to the copied object and its direct or indirect members +/// in copy constructors and copy assignment operators. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-mutating-copy.html +class MutatingCopyCheck : public ClangTidyCheck { +public: + MutatingCopyCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MUTATINGCOPYCHECK_H Index: clang-tools-extra/clang-tidy/misc/MutatingCopyCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/misc/MutatingCopyCheck.cpp @@ -0,0 +1,61 @@ +//===--- MutatingCopyCheck.cpp - clang-tidy -------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "MutatingCopyCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +static constexpr llvm::StringLiteral SourceDeclName = "ChangedPVD"; +static constexpr llvm::StringLiteral MutatingOperatorName = "MutatingOp"; + +void MutatingCopyCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + + const auto MemberExprOrSourceObject = anyOf( + memberExpr(), declRefExpr(to(decl(equalsBoundNode(SourceDeclName))))); + + const auto IsPartOfSource = + allOf(unless(hasDescendant(expr(unless(MemberExprOrSourceObject)))), + MemberExprOrSourceObject); + + const auto IsSourceMutatingAssignment = + expr(anyOf(binaryOperator(isAssignmentOperator(), hasLHS(IsPartOfSource)) + .bind(MutatingOperatorName), + cxxOperatorCallExpr(isAssignmentOperator(), + hasArgument(0, IsPartOfSource)) + .bind(MutatingOperatorName))); + + const auto MutatesSource = allOf( + hasParameter( + 0, parmVarDecl(hasType(lValueReferenceType())).bind(SourceDeclName)), + forEachDescendant(IsSourceMutatingAssignment)); + + Finder->addMatcher(cxxConstructorDecl(isCopyConstructor(), MutatesSource), + this); + + Finder->addMatcher(cxxMethodDecl(isCopyAssignmentOperator(), MutatesSource), + this); +} + +void MutatingCopyCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *Assignment = + Result.Nodes.getNodeAs<Expr>(MutatingOperatorName)) { + diag(Assignment->getBeginLoc(), "mutating copied object"); + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModuleRegistry.h" #include "DefinitionsInHeadersCheck.h" #include "MisplacedConstCheck.h" +#include "MutatingCopyCheck.h" #include "NewDeleteOverloadsCheck.h" #include "NonCopyableObjects.h" #include "NonPrivateMemberVariablesInClassesCheck.h" @@ -33,6 +34,8 @@ CheckFactories.registerCheck<DefinitionsInHeadersCheck>( "misc-definitions-in-headers"); CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const"); + CheckFactories.registerCheck<MutatingCopyCheck>( + "misc-mutating-copy"); CheckFactories.registerCheck<NewDeleteOverloadsCheck>( "misc-new-delete-overloads"); CheckFactories.registerCheck<NonCopyableObjectsCheck>( Index: clang-tools-extra/clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -4,6 +4,7 @@ DefinitionsInHeadersCheck.cpp MiscTidyModule.cpp MisplacedConstCheck.cpp + MutatingCopyCheck.cpp NewDeleteOverloadsCheck.cpp NonCopyableObjects.cpp NonPrivateMemberVariablesInClassesCheck.cpp
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
