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
