[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 =
----------------
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);
```

================
Comment at: clang-tidy/cppcoreguidelines/SlicingCheck.cpp:84
@@ +83,3 @@
+      diag(Call.getExprLoc(),
+           "slicing object from type %0 to %1 discards override %2")
+          << &DerivedDecl << &BaseDecl << Method;
----------------
There are two diagnostic messages in the code. For subclasses with override base class methods and extra member, this will print two messages, which is a bit of redundant.

================
Comment at: clang-tidy/cppcoreguidelines/SlicingCheck.cpp:94
@@ +93,3 @@
+    CXXRecordDecl *BaseRecord =
+        cast_or_null<CXXRecordDecl>(BaseRecordType->getDecl()->getDefinition());
+    if (!BaseRecord)
----------------
The code can be simplified like:

```
if (const auto *BR = cast_or_null<CXXRecordDecl>(BaseRecordType->getDecl()->getDefinition())) {
  DiagnoseSlicedOverriddenMethods(Call, *BR, BaseDecl); 
}
```

================
Comment at: clang-tidy/cppcoreguidelines/SlicingCheck.cpp:112
@@ +111,3 @@
+  // We're looking through all the methods in the derived class and see if they
+  // override some method in the base class.
+  // It's not enough to just test whether the class is polymorphic because we
----------------
s/method/methods


http://reviews.llvm.org/D21992