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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
