[clang-tools-extra] r276413 - Revert "[clang-tidy] new cppcoreguidelines-slicing"

Clement Courbet via cfe-commits cfe-commits at lists.llvm.org
Fri Jul 22 06:33:52 PDT 2016


Author: courbet
Date: Fri Jul 22 08:33:51 2016
New Revision: 276413

URL: http://llvm.org/viewvc/llvm-project?rev=276413&view=rev
Log:
Revert "[clang-tidy] new cppcoreguidelines-slicing"

Tests fail on clang-x64-ninja-win7 due to too narrow expectation.

Removed:
    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=276413&r1=276412&r2=276413&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt Fri Jul 22 08:33:51 2016
@@ -13,7 +13,6 @@ 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=276413&r1=276412&r2=276413&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp Fri Jul 22 08:33:51 2016
@@ -22,7 +22,6 @@
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
-#include "SlicingCheck.h"
 
 namespace clang {
 namespace tidy {
@@ -54,8 +53,6 @@ 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");
   }

Removed: 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=276412&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.cpp (removed)
@@ -1,135 +0,0 @@
-//===--- 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

Removed: 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=276412&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.h (original)
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SlicingCheck.h (removed)
@@ -1,45 +0,0 @@
-//===--- 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=276413&r1=276412&r2=276413&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Fri Jul 22 08:33:51 2016
@@ -59,10 +59,7 @@ The improvements are...
 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.
+The improvements are...
 
 Improvements to include-fixer
 -----------------------------

Removed: 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=276412&view=auto
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-slicing.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-slicing.rst (removed)
@@ -1,23 +0,0 @@
-.. 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

Removed: 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=276412&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-slicing.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-slicing.cpp (removed)
@@ -1,100 +0,0 @@
-// 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'
-}




More information about the cfe-commits mailing list