On Fri, Jul 22, 2016 at 2:42 PM, Clement Courbet via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> Author: courbet > Date: Fri Jul 22 07:42:19 2016 > New Revision: 276408 > > URL: http://llvm.org/viewvc/llvm-project?rev=276408&view=rev > Log: > [clang-tidy] new cppcoreguidelines-slicing > > Flags slicing of member variables or vtable. See: > > > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es63-dont-slice > > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c145-access-polymorphic-objects-through-pointers-and-references > > Differential revision: > http://reviews.llvm.org/D21974 I guess, you meant https://reviews.llvm.org/D21992 > > Added: > clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.cpp > clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.h > > clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-slicing.rst > clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-slicing.cpp > Modified: > clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt > > clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp > clang-tools-extra/trunk/docs/ReleaseNotes.rst > > Modified: > clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt?rev=276408&r1=276407&r2=276408&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt > (original) > +++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt > Fri Jul 22 07:42:19 2016 > @@ -13,6 +13,7 @@ add_clang_library(clangTidyCppCoreGuidel > ProTypeStaticCastDowncastCheck.cpp > ProTypeUnionAccessCheck.cpp > ProTypeVarargCheck.cpp > + SlicingCheck.cpp > > LINK_LIBS > clangAST > > Modified: > clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp?rev=276408&r1=276407&r2=276408&view=diff > > ============================================================================== > --- > clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp > (original) > +++ > clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp > Fri Jul 22 07:42:19 2016 > @@ -22,6 +22,7 @@ > #include "ProTypeStaticCastDowncastCheck.h" > #include "ProTypeUnionAccessCheck.h" > #include "ProTypeVarargCheck.h" > +#include "SlicingCheck.h" > > namespace clang { > namespace tidy { > @@ -53,6 +54,8 @@ public: > "cppcoreguidelines-pro-type-union-access"); > CheckFactories.registerCheck<ProTypeVarargCheck>( > "cppcoreguidelines-pro-type-vararg"); > + CheckFactories.registerCheck<SlicingCheck>( > + "cppcoreguidelines-slicing"); > CheckFactories.registerCheck<misc::UnconventionalAssignOperatorCheck>( > "cppcoreguidelines-c-copy-assignment-signature"); > } > > Added: > clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.cpp?rev=276408&view=auto > > ============================================================================== > --- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.cpp > (added) > +++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.cpp > Fri Jul 22 07:42:19 2016 > @@ -0,0 +1,135 @@ > +//===--- SlicingCheck.cpp - > clang-tidy-------------------------------------===// > +// > +// The LLVM Compiler Infrastructure > +// > +// This file is distributed under the University of Illinois Open Source > +// License. See LICENSE.TXT for details. > +// > > +//===----------------------------------------------------------------------===// > + > +#include "SlicingCheck.h" > +#include "clang/AST/ASTContext.h" > +#include "clang/AST/RecordLayout.h" > +#include "clang/ASTMatchers/ASTMatchFinder.h" > +#include "clang/ASTMatchers/ASTMatchers.h" > + > +using namespace clang::ast_matchers; > + > +namespace clang { > +namespace tidy { > +namespace cppcoreguidelines { > + > +void SlicingCheck::registerMatchers(MatchFinder *Finder) { > + // When we see: > + // class B : public A { ... }; > + // A a; > + // B b; > + // a = b; > + // The assignment is OK if: > + // - the assignment operator is defined as taking a B as second > parameter, > + // or > + // - B does not define any additional members (either variables or > + // overrides) wrt A. > + // > + // The same holds for copy ctor calls. This also captures stuff like: > + // void f(A a); > + // f(b); > + > + // Helpers. > + const auto OfBaseClass = ofClass(cxxRecordDecl().bind("BaseDecl")); > + const auto IsDerivedFromBaseDecl = > + cxxRecordDecl(isDerivedFrom(equalsBoundNode("BaseDecl"))) > + .bind("DerivedDecl"); > + const auto HasTypeDerivedFromBaseDecl = > + anyOf(hasType(IsDerivedFromBaseDecl), > + hasType(references(IsDerivedFromBaseDecl))); > + const auto IsWithinDerivedCtor = > + > hasParent(cxxConstructorDecl(ofClass(equalsBoundNode("DerivedDecl")))); > + > + // Assignement slicing: "a = b;" and "a = std::move(b);" variants. > + const auto SlicesObjectInAssignment = > + callExpr(callee(cxxMethodDecl(anyOf(isCopyAssignmentOperator(), > + isMoveAssignmentOperator()), > + OfBaseClass)), > + hasArgument(1, HasTypeDerivedFromBaseDecl)); > + > + // Construction slicing: "A a{b};" and "f(b);" variants. Note that in > case of > + // slicing the letter will create a temporary and therefore call a ctor. > + const auto SlicesObjectInCtor = cxxConstructExpr( > + hasDeclaration(cxxConstructorDecl( > + anyOf(isCopyConstructor(), isMoveConstructor()), OfBaseClass)), > + hasArgument(0, HasTypeDerivedFromBaseDecl), > + // We need to disable matching on the call to the base copy/move > + // constructor in DerivedDecl's constructors. > + unless(IsWithinDerivedCtor)); > + > + Finder->addMatcher( > + expr(anyOf(SlicesObjectInAssignment, > SlicesObjectInCtor)).bind("Call"), > + this); > +} > + > +/// Warns on methods overridden in DerivedDecl with respect to BaseDecl. > +/// FIXME: this warns on all overrides outside of the sliced path in case > of > +/// multiple inheritance. > +void SlicingCheck::DiagnoseSlicedOverriddenMethods( > + const Expr &Call, const CXXRecordDecl &DerivedDecl, > + const CXXRecordDecl &BaseDecl) { > + if (DerivedDecl.getCanonicalDecl() == BaseDecl.getCanonicalDecl()) > + return; > + for (const auto &Method : DerivedDecl.methods()) { > + // Virtual destructors are OK. We're ignoring constructors since they > are > + // tagged as overrides. > + if (isa<CXXConstructorDecl>(Method) || isa<CXXDestructorDecl>(Method)) > + continue; > + if (Method->size_overridden_methods() > 0) { > + diag(Call.getExprLoc(), > + "slicing object from type %0 to %1 discards override %2") > + << &DerivedDecl << &BaseDecl << Method; > + } > + } > + // Recursively process bases. > + for (const auto &Base : DerivedDecl.bases()) { > + if (const auto *BaseRecordType = Base.getType()->getAs<RecordType>()) > { > + if (const auto *BaseRecord = cast_or_null<CXXRecordDecl>( > + BaseRecordType->getDecl()->getDefinition())) > + DiagnoseSlicedOverriddenMethods(Call, *BaseRecord, BaseDecl); > + } > + } > +} > + > +void SlicingCheck::check(const MatchFinder::MatchResult &Result) { > + const auto *BaseDecl = > Result.Nodes.getNodeAs<CXXRecordDecl>("BaseDecl"); > + const auto *DerivedDecl = > + Result.Nodes.getNodeAs<CXXRecordDecl>("DerivedDecl"); > + const auto *Call = Result.Nodes.getNodeAs<Expr>("Call"); > + assert(BaseDecl != nullptr); > + assert(DerivedDecl != nullptr); > + assert(Call != nullptr); > + > + // Warn when slicing the vtable. > + // We're looking through all the methods in the derived class and see > if they > + // override some methods in the base class. > + // It's not enough to just test whether the class is polymorphic > because we > + // would be fine slicing B to A if no method in B (or its bases) > overrides > + // anything in A: > + // class A { virtual void f(); }; > + // class B : public A {}; > + // because in that case calling A::f is the same as calling B::f. > + DiagnoseSlicedOverriddenMethods(*Call, *DerivedDecl, *BaseDecl); > + > + // Warn when slicing member variables. > + const auto &BaseLayout = > + BaseDecl->getASTContext().getASTRecordLayout(BaseDecl); > + const auto &DerivedLayout = > + DerivedDecl->getASTContext().getASTRecordLayout(DerivedDecl); > + const auto StateSize = DerivedLayout.getDataSize() - > BaseLayout.getDataSize(); > + if (StateSize.isPositive()) { > + diag(Call->getExprLoc(), "slicing object from type %0 to %1 discards " > + "%2*sizeof(char) bytes of state") > + << DerivedDecl << BaseDecl << > static_cast<int>(StateSize.getQuantity()); > + } > +} > + > +} // namespace cppcoreguidelines > +} // namespace tidy > +} // namespace clang > > Added: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.h > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.h?rev=276408&view=auto > > ============================================================================== > --- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.h > (added) > +++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.h > Fri Jul 22 07:42:19 2016 > @@ -0,0 +1,45 @@ > +//===--- SlicingCheck.h - clang-tidy-----------------------------*- C++ > -*-===// > +// > +// The LLVM Compiler Infrastructure > +// > +// This file is distributed under the University of Illinois Open Source > +// License. See LICENSE.TXT for details. > +// > > +//===----------------------------------------------------------------------===// > + > +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SLICING_H > +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SLICING_H > + > +#include "../ClangTidy.h" > + > +namespace clang { > +namespace tidy { > +namespace cppcoreguidelines { > + > +/// Flags slicing (incomplete copying of an object's state) of member > variables > +/// or vtable. See: > +/// - > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es63-dont-slice > +/// for the former, and > +/// - > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c145-access-polymorphic-objects-through-pointers-and-references > +/// for the latter > +/// > +/// For the user-facing documentation see: > +/// > http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-slicing.html > +class SlicingCheck : public ClangTidyCheck { > +public: > + SlicingCheck(StringRef Name, ClangTidyContext *Context) > + : ClangTidyCheck(Name, Context) {} > + void registerMatchers(ast_matchers::MatchFinder *Finder) override; > + void check(const ast_matchers::MatchFinder::MatchResult &Result) > override; > + > +private: > + void DiagnoseSlicedOverriddenMethods(const Expr &call, > + const CXXRecordDecl &DerivedDecl, > + const CXXRecordDecl &BaseDecl); > +}; > + > +} // namespace cppcoreguidelines > +} // namespace tidy > +} // namespace clang > + > +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SLICING_H > > Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=276408&r1=276407&r2=276408&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original) > +++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Fri Jul 22 07:42:19 2016 > @@ -59,7 +59,10 @@ The improvements are... > Improvements to clang-tidy > -------------------------- > > -The improvements are... > +- New `cppcoreguidelines-slicing > + < > http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-slicing.html>`_ > check > + > + Flags slicing of member variables or vtable. > > Improvements to include-fixer > ----------------------------- > > Added: > clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-slicing.rst > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-slicing.rst?rev=276408&view=auto > > ============================================================================== > --- > clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-slicing.rst > (added) > +++ > clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-slicing.rst > Fri Jul 22 07:42:19 2016 > @@ -0,0 +1,23 @@ > +.. title:: clang-tidy - cppcoreguidelines-slicing > + > +cppcoreguidelines-slicing > +========================= > + > +Flags slicing of member variables or vtable. Slicing happens when copying > a > +derived object into a base object: the members of the derived object (both > +member variables and virtual member functions) will be discarded. > +This can be misleading especially for member function slicing, for > example: > + > +.. code:: c++ > + > + struct B { int a; virtual int f(); }; > + struct D : B { int b; int f() override; }; > + void use(B b) { // Missing reference, intended ? > + b.f(); // Calls B::f. > + } > + D d; > + use(d); // Slice. > + > +See the relevant CppCoreGuidelines sections for details: > + > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es63-dont-slice > + > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c145-access-polymorphic-objects-through-pointers-and-references > > Added: > clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-slicing.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-slicing.cpp?rev=276408&view=auto > > ============================================================================== > --- clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-slicing.cpp > (added) > +++ clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-slicing.cpp > Fri Jul 22 07:42:19 2016 > @@ -0,0 +1,100 @@ > +// RUN: %check_clang_tidy %s cppcoreguidelines-slicing %t > + > +class Base { > + int i; > + void f() {} > + virtual void g() {} > +}; > + > +class DerivedWithMemberVariables : public Base { > + void f(); > + int j; > +}; > + > +class TwiceDerivedWithNoMemberVariables : public > DerivedWithMemberVariables { > + void f(); > +}; > + > +class DerivedWithOverride : public Base { > + void f(); > + void g() override {} > +}; > + > +class TwiceDerivedWithNoOverride : public DerivedWithOverride { > + void f(); > +}; > + > +void TakesBaseByValue(Base base); > + > +DerivedWithMemberVariables ReturnsDerived(); > + > +void positivesWithMemberVariables() { > + DerivedWithMemberVariables b; > + Base a{b}; > + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: slicing object from type > 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) > bytes of state [cppcoreguidelines-slicing] > + a = b; > + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type > 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) > bytes of state > + TakesBaseByValue(b); > + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: slicing object from type > 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) > bytes of state > + > + TwiceDerivedWithNoMemberVariables c; > + a = c; > + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type > 'TwiceDerivedWithNoMemberVariables' to 'Base' discards > {{[0-9]*}}*sizeof(char) bytes of state > + > + a = ReturnsDerived(); > + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type > 'DerivedWithMemberVariables' to 'Base' discards 4*sizeof(char) bytes of > state > +} > + > +void positivesWithOverride() { > + DerivedWithOverride b; > + Base a{b}; > + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: slicing object from type > 'DerivedWithOverride' to 'Base' discards override 'g' > + a = b; > + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type > 'DerivedWithOverride' to 'Base' discards override 'g' > + TakesBaseByValue(b); > + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: slicing object from type > 'DerivedWithOverride' to 'Base' discards override 'g' > + > + TwiceDerivedWithNoOverride c; > + a = c; > + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type > 'DerivedWithOverride' to 'Base' discards override 'g' > +} > + > +void TakesBaseByReference(Base &base); > + > +class DerivedThatAddsVirtualH : public Base { > + virtual void h(); > +}; > + > +class DerivedThatOverridesH : public DerivedThatAddsVirtualH { > + void h() override; > +}; > + > +void negatives() { > + // OK, simple copying from the same type. > + Base a; > + TakesBaseByValue(a); > + DerivedWithMemberVariables b; > + DerivedWithMemberVariables c{b}; > + b = c; > + > + // OK, derived type does not have extra state. > + TwiceDerivedWithNoMemberVariables d; > + DerivedWithMemberVariables e{d}; > + e = d; > + > + // OK, derived does not override any method. > + TwiceDerivedWithNoOverride f; > + DerivedWithOverride g{f}; > + g = f; > + > + // OK, no copying. > + TakesBaseByReference(d); > + TakesBaseByReference(f); > + > + // Derived type overrides methods, but these methods are not in the > base type, > + // so cannot be called accidentally. Right now this triggers, but we > might > + // want to allow it. > + DerivedThatOverridesH h; > + a = h; > + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type > 'DerivedThatOverridesH' to 'Base' discards override 'h' > +} > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
