[clang-tidy] Fix false-positive in cppcoreguidelines-slicing

[PiotrZSL]

Summary

When warning would be emitted in constructor for virtual base class
initialization.

Fixes: https://github.com/llvm/llvm-project/issues/31187

[PiotrZSL]

Ready for review.

[PiotrZSL]

Ping, Rebase

[PiotrZSL]

Rebase due to broken baseline

[carlosgalvezp]

Looks good, just a small comment!

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/slicing.cpp

102

Please briefly document this test case, i.e. why in this case this is not considered slicing and the check should not warn. It would be good to also mention the Github issue for traceability.

[PiotrZSL]

Review comments fix

[PiotrZSL]

This comment was removed by PiotrZSL.

[carlosgalvezp]

LGTM, thanks for the fix!

clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp

void SlicingCheck::registerMatchers(MatchFinder *Finder) {
   //  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"))));
+  const auto IsCallToBaseClass = hasParent(cxxConstructorDecl(
+      ofClass(isSameOrDerivedFrom(equalsBoundNode("DerivedDecl"))),
+      hasAnyConstructorInitializer(allOf(
+          isBaseInitializer(), withInitializer(equalsBoundNode("Call"))))));
 
   // Assignment slicing: "a = b;" and "a = std::move(b);" variants.
   const auto SlicesObjectInAssignment =
-      callExpr(callee(cxxMethodDecl(anyOf(isCopyAssignmentOperator(),
+      callExpr(expr().bind("Call"),
+               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(
+      expr().bind("Call"),
       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));
+      unless(IsCallToBaseClass));
 
-  Finder->addMatcher(traverse(TK_AsIs, expr(anyOf(SlicesObjectInAssignment,
-                                                  SlicesObjectInCtor))
-                                           .bind("Call")),
-                     this);
+  Finder->addMatcher(
+      traverse(TK_AsIs, expr(SlicesObjectInAssignment).bind("Call")), this);
+  Finder->addMatcher(traverse(TK_AsIs, SlicesObjectInCtor), 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) {

clang-tools-extra/docs/ReleaseNotes.rst

- Fixed a false positive in :doc:`readability-container-size-empty
   <clang-tidy/checks/readability/container-size-empty>` check when comparing
   ``std::array`` objects to default constructed ones. The behavior for this and
   other relevant classes can now be configured with a new option.
 
 - Improved :doc:`bugprone-too-small-loop-variable
   <clang-tidy/checks/bugprone/too-small-loop-variable>` check. Basic support
   for bit-field and integer members as a loop variable or upper limit were added.
 
-- Improved :doc:`readability-magic-numbers 
+- Improved :doc:`readability-magic-numbers
   <clang-tidy/checks/readability/magic-numbers>` check, now allows for
   magic numbers in type aliases such as ``using`` and ``typedef`` declarations if
   the new ``IgnoreTypeAliases`` option is set to true.
 
+- Fixed a false positive in :doc:`cppcoreguidelines-slicing
+  <clang-tidy/checks/cppcoreguidelines/slicing>` check when warning would be
+  emitted in constructor for virtual base class initialization.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/slicing.cpp

void negatives() {
 
   // 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'
 }
+
+namespace PR31187 {
+// Don't warn when calling constructor of base virtual class, from
+// initialization list of derived class constructor.
+
+struct BaseA {
+virtual ~BaseA() {}
+virtual void foo() {}
+
+int i;
+};
+
+struct BaseB : virtual BaseA {
+virtual void foo() {}
+};
+
+struct ClassWithVirtualBases : BaseB {
+  ClassWithVirtualBases(const BaseB& other) : BaseA(other), BaseB(other) {}
+  ClassWithVirtualBases(const ClassWithVirtualBases& other) : BaseA(other), BaseB(other) {}
+};
+
+}