courbet updated this revision to Diff 65028.
courbet marked an inline comment as done.
https://reviews.llvm.org/D21992
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tidy/cppcoreguidelines/SlicingCheck.cpp
clang-tidy/cppcoreguidelines/SlicingCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-slicing.rst
test/clang-tidy/cppcoreguidelines-slicing.cpp
Index: test/clang-tidy/cppcoreguidelines-slicing.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-slicing.cpp
@@ -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'
+}
Index: docs/clang-tidy/checks/cppcoreguidelines-slicing.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-slicing.rst
@@ -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
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -59,7 +59,10 @@
Improvements to clang-tidy
--------------------------
-...
+- New `cppcoreguidelines-slicing
+ <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-slicing.html>`_ check
+
+ Flags slicing of member variables or vtable.
Clang-tidy changes from 3.9 to 4.0
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Index: clang-tidy/cppcoreguidelines/SlicingCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/cppcoreguidelines/SlicingCheck.h
@@ -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
Index: clang-tidy/cppcoreguidelines/SlicingCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/cppcoreguidelines/SlicingCheck.cpp
@@ -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
Index: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===================================================================
--- clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -22,6 +22,7 @@
#include "ProTypeStaticCastDowncastCheck.h"
#include "ProTypeUnionAccessCheck.h"
#include "ProTypeVarargCheck.h"
+#include "SlicingCheck.h"
namespace clang {
namespace tidy {
@@ -53,6 +54,8 @@
"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");
}
Index: clang-tidy/cppcoreguidelines/CMakeLists.txt
===================================================================
--- clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -13,6 +13,7 @@
ProTypeStaticCastDowncastCheck.cpp
ProTypeUnionAccessCheck.cpp
ProTypeVarargCheck.cpp
+ SlicingCheck.cpp
LINK_LIBS
clangAST
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
