[clang-tools-extra] r276408 - [clang-tidy] new cppcoreguidelines-slicing
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 25 08:26:51 PDT 2016
On Fri, Jul 22, 2016 at 2:42 PM, Clement Courbet via cfe-commits <
cfe-commits at 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160725/2dabefe8/attachment-0001.html>
More information about the cfe-commits
mailing list