gbencze updated this revision to Diff 229925.
gbencze added a comment.

Formatting changes (curly braces, newline at EOF)
Remove incorrect flag from test


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70052/new/

https://reviews.llvm.org/D70052

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp
  clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-oop58-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp
===================================================================
--- /dev/null
+++ 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
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
@@ -105,6 +105,7 @@
    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
    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>
    clang-analyzer-core.DynamicTypePropagation
Index: clang-tools-extra/docs/clang-tidy/checks/cert-oop58-cpp.rst
===================================================================
--- /dev/null
+++ 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>`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -88,6 +88,12 @@
   Without the null terminator it can result in undefined behaviour when the
   string is read.
 
+- 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.
 
Index: clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.h
===================================================================
--- /dev/null
+++ 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
Index: clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp
===================================================================
--- /dev/null
+++ 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
Index: clang-tools-extra/clang-tidy/cert/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/cert/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/cert/CMakeLists.txt
@@ -6,6 +6,7 @@
   DontModifyStdNamespaceCheck.cpp
   FloatLoopCounter.cpp
   LimitedRandomnessCheck.cpp
+  MutatingCopyCheck.cpp
   PostfixOperatorCheck.cpp
   ProperlySeededRandomGeneratorCheck.cpp
   SetLongJmpCheck.cpp
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
@@ -21,6 +21,7 @@
 #include "DontModifyStdNamespaceCheck.h"
 #include "FloatLoopCounter.h"
 #include "LimitedRandomnessCheck.h"
+#include "MutatingCopyCheck.h"
 #include "PostfixOperatorCheck.h"
 #include "ProperlySeededRandomGeneratorCheck.h"
 #include "SetLongJmpCheck.h"
@@ -52,6 +53,8 @@
         "cert-oop11-cpp");
     CheckFactories.registerCheck<bugprone::UnhandledSelfAssignmentCheck>(
         "cert-oop54-cpp");
+    CheckFactories.registerCheck<MutatingCopyCheck>(
+        "cert-oop58-cpp");
     // ERR
     CheckFactories.registerCheck<misc::ThrowByValueCatchByReferenceCheck>(
         "cert-err09-cpp");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to