[PATCH] D21992: [clang-tidy] new cppcoreguidelines-slicing

[Haojian Wu via cfe-commits]

hokein added inline comments.

================
Comment at: clang-tidy/cppcoreguidelines/SlicingCheck.cpp:48
@@ +47,3 @@
+
+  // Assignement slicing: "a = b;" and "a = std::move(b);" variants.
+  const auto SlicesObjectInAssignment =
----------------
courbet wrote:
> hokein wrote:
> > Looks like you are missing some cases here, like assigning a Subclass object from a function call to a Baseclass object, passing a Subclass object to a function whose parameter is a BaseClass.
> > 
> > But I think it's fine to implement in a separate patch, but you'd better to add a TODO here.
> > 
> > ```
> > SubClass f1();
> > BaseClass a = f1();
> > 
> > void f1(BaseClass a);
> > SubClass sc;
> > f1(sc);
> > ```
> Actually these will still create a CXXConstructExpr in the AST, e.g for case (2):
> 
> ```
> CallExpr 0x3d6e560 </usr/local/google/home/courbet/test2.cc:10:3, col:6> 'void'
> |-ImplicitCastExpr 0x3d6e548 <col:3> 'void (*)(class A)' <FunctionToPointerDecay>
> | `-DeclRefExpr 0x3d6e4f8 <col:3> 'void (class A)' lvalue Function 0x3d66550 'g' 'void (class A)'
> `-CXXConstructExpr 0x3d6e5c8 <col:5> 'class A' 'void (const class A &) throw()'
>   `-ImplicitCastExpr 0x3d6e5b0 <col:5> 'const class A' lvalue <NoOp>
>     `-ImplicitCastExpr 0x3d6e590 <col:5> 'class A' lvalue <DerivedToBase (A)>
>       `-DeclRefExpr 0x3d6e4d0 <col:5> 'class B' lvalue Var 0x3d6e300 'b' 'class B'
> ```
> 
> I alreagy have a test to case (2) and I've added one for case (1).
> 
Oh, I see it now. Sorry for missing them and thanks for explanation.  It'd be more clear to if you can update comments at `SlicesObjectInCtor`.

================
Comment at: clang-tidy/cppcoreguidelines/SlicingCheck.cpp:85
@@ +84,3 @@
+           "slicing object from type %0 to %1 discards override %2")
+          << &DerivedDecl << &BaseDecl << Method;
+    }
----------------
Maybe alexfh@ has idea about this. Output multiple warning messages in a same location is not the pattern of clang-tidy message. I think we should combine three message into exact one, something like `"slicing object from type 'B' to 'A' discards override 'f', 'g', 4*sizeof(char) bytes of state"`.



================
Comment at: docs/clang-tidy/checks/cppcoreguidelines-slicing.rst:24
@@ +23,2 @@
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c145-access-polymorphic-objects-through-pointers-and-references
+
----------------
There is an extra blank line at the bottom.

================
Comment at: test/clang-tidy/cppcoreguidelines-slicing.cpp:95
@@ +94,3 @@
+  // Derived type overrides methods, but these methods are not in the base type,
+  // so cannot be called accidentally. Righ tnow this triggers, but we might
+  // want to allow it.
----------------
s/tnow/now


http://reviews.llvm.org/D21992