Author: Gabor Bencze Date: 2019-12-15T16:30:14+01:00 New Revision: bbc9f6c2ef074668374e06a5de471413afd2ee4b
URL: https://github.com/llvm/llvm-project/commit/bbc9f6c2ef074668374e06a5de471413afd2ee4b DIFF: https://github.com/llvm/llvm-project/commit/bbc9f6c2ef074668374e06a5de471413afd2ee4b.diff LOG: [clang-tidy] Add cert-oop58-cpp check The check warns when (a member of) the copied object is assigned to in a copy constructor or copy assignment operator. Based on https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP58-CPP.+Copy+operations+must+not+mutate+the+source+object Differential Revision: https://reviews.llvm.org/D70052 Added: clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.h clang-tools-extra/docs/clang-tidy/checks/cert-oop58-cpp.rst clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp Modified: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp clang-tools-extra/clang-tidy/cert/CMakeLists.txt clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/list.rst Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp index 3130dad68ffd..26f0bdab5345 100644 --- a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp @@ -23,6 +23,7 @@ #include "DontModifyStdNamespaceCheck.h" #include "FloatLoopCounter.h" #include "LimitedRandomnessCheck.h" +#include "MutatingCopyCheck.h" #include "PostfixOperatorCheck.h" #include "ProperlySeededRandomGeneratorCheck.h" #include "SetLongJmpCheck.h" @@ -69,6 +70,8 @@ class CERTModule : public ClangTidyModule { "cert-oop11-cpp"); CheckFactories.registerCheck<bugprone::UnhandledSelfAssignmentCheck>( "cert-oop54-cpp"); + CheckFactories.registerCheck<MutatingCopyCheck>( + "cert-oop58-cpp"); // C checkers // DCL diff --git a/clang-tools-extra/clang-tidy/cert/CMakeLists.txt b/clang-tools-extra/clang-tidy/cert/CMakeLists.txt index 0363db7cf02d..66ea2a13acdd 100644 --- a/clang-tools-extra/clang-tidy/cert/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/cert/CMakeLists.txt @@ -7,6 +7,7 @@ add_clang_library(clangTidyCERTModule DontModifyStdNamespaceCheck.cpp FloatLoopCounter.cpp LimitedRandomnessCheck.cpp + MutatingCopyCheck.cpp PostfixOperatorCheck.cpp ProperlySeededRandomGeneratorCheck.cpp SetLongJmpCheck.cpp diff --git a/clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp b/clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp new file mode 100644 index 000000000000..a20a890afc70 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp @@ -0,0 +1,83 @@ +//===--- MutatingCopyCheck.cpp - clang-tidy -------------------------------===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +#include "MutatingCopyCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace cert { + +static constexpr llvm::StringLiteral SourceDeclName = "ChangedPVD"; +static constexpr llvm::StringLiteral MutatingOperatorName = "MutatingOp"; +static constexpr llvm::StringLiteral MutatingCallName = "MutatingCall"; + +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 MemberExprOrSelf = anyOf(memberExpr(), cxxThisExpr()); + + const auto IsPartOfSelf = allOf( + unless(hasDescendant(expr(unless(MemberExprOrSelf)))), MemberExprOrSelf); + + const auto IsSelfMutatingAssignment = + expr(anyOf(binaryOperator(isAssignmentOperator(), hasLHS(IsPartOfSelf)), + cxxOperatorCallExpr(isAssignmentOperator(), + hasArgument(0, IsPartOfSelf)))); + + const auto IsSelfMutatingMemberFunction = + functionDecl(hasBody(hasDescendant(IsSelfMutatingAssignment))); + + const auto IsSourceMutatingMemberCall = + cxxMemberCallExpr(on(IsPartOfSource), + callee(IsSelfMutatingMemberFunction)) + .bind(MutatingCallName); + + const auto MutatesSource = allOf( + hasParameter( + 0, parmVarDecl(hasType(lValueReferenceType())).bind(SourceDeclName)), + anyOf(forEachDescendant(IsSourceMutatingAssignment), + forEachDescendant(IsSourceMutatingMemberCall))); + + Finder->addMatcher(cxxConstructorDecl(isCopyConstructor(), MutatesSource), + this); + + Finder->addMatcher(cxxMethodDecl(isCopyAssignmentOperator(), MutatesSource), + this); +} + +void MutatingCopyCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *MemberCall = + Result.Nodes.getNodeAs<CXXMemberCallExpr>(MutatingCallName)) + diag(MemberCall->getBeginLoc(), "call mutates copied object"); + else if (const auto *Assignment = + Result.Nodes.getNodeAs<Expr>(MutatingOperatorName)) + diag(Assignment->getBeginLoc(), "mutating copied object"); +} + +} // namespace cert +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.h b/clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.h new file mode 100644 index 000000000000..0efba6aff3cb --- /dev/null +++ b/clang-tools-extra/clang-tidy/cert/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_CERT_MUTATINGCOPYCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_MUTATINGCOPYCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace cert { + +/// 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/cert-oop58-cpp.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 cert +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_MUTATINGCOPYCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index ec56c6d6a784..a150cc6bf824 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -105,6 +105,12 @@ Improvements to clang-tidy :doc:`bugprone-bad-signal-to-kill-thread <clang-tidy/checks/bugprone-bad-signal-to-kill-thread>` was added. +- New :doc:`cert-oop58-cpp + <clang-tidy/checks/cert-oop58-cpp>` check. + + Finds assignments to the copied object and its direct or indirect members + in copy constructors and copy assignment operators. + - New :doc:`cppcoreguidelines-init-variables <clang-tidy/checks/cppcoreguidelines-init-variables>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cert-oop58-cpp.rst b/clang-tools-extra/docs/clang-tidy/checks/cert-oop58-cpp.rst new file mode 100644 index 000000000000..399fb1b7e927 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cert-oop58-cpp.rst @@ -0,0 +1,11 @@ +.. title:: clang-tidy - cert-mutating-copy + +cert-oop58-cpp +============== + +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>`_. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index a005ed8ef0a8..1bd1a55bc791 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -107,6 +107,7 @@ Clang-Tidy Checks 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> + cert-oop58-cpp cert-pos44-c (redirects to bugprone-bad-signal-to-kill-thread) <cert-pos44-c> clang-analyzer-core.CallAndMessage (redirects to https://clang.llvm.org/docs/analyzer/checkers) <clang-analyzer-core.CallAndMessage> clang-analyzer-core.DivideZero (redirects to https://clang.llvm.org/docs/analyzer/checkers) <clang-analyzer-core.DivideZero> diff --git a/clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp b/clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp new file mode 100644 index 000000000000..223248cb8847 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp @@ -0,0 +1,149 @@ +// RUN: %check_clang_tidy %s cert-oop58-cpp %t + +// 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 + +namespace test_mutating_member_function { +class F { + int a; + +public: + void bad_func() { a = 12; } + void fine_func() const; + void fine_func_2(int x) { x = 5; } + void questionable_func(); + + F(F &other) : a(other.a) { + this->bad_func(); // no-warning: mutating this is allowed + + other.bad_func(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: call mutates copied object + + other.fine_func(); + other.fine_func_2(42); + other.questionable_func(); + } +}; +} // namespace test_mutating_member_function + +namespace test_mutating_function_on_nested_object { +struct S { + int x; + void mutate(int y) { + x = y; + } +}; + +class G { + S s; + G(G &other) { + s.mutate(0); // no-warning: mutating this is allowed + + other.s.mutate(0); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: call mutates copied object + } +}; +} // namespace test_mutating_function_on_nested_object _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
