[clang-tools-extra] bbc9f6c - [clang-tidy] Add cert-oop58-cpp check
Gabor Bencze via cfe-commits
cfe-commits at lists.llvm.org
Sun Dec 15 07:30:31 PST 2019
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
More information about the cfe-commits
mailing list