[PATCH] D144206: [clang-tidy] Fix false-positive in cppcoreguidelines-slicing

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 504377.
PiotrZSL marked an inline comment as done.
PiotrZSL added a comment.

Review comments fix


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144206/new/

https://reviews.llvm.org/D144206

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/slicing.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/slicing.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/slicing.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/slicing.cpp
@@ -98,3 +98,25 @@
   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) {}
+};
+
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -199,11 +199,15 @@
   <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
 ^^^^^^^^^^^^^^
 
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp
@@ -40,12 +40,15 @@
   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));
@@ -53,17 +56,17 @@
   // 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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D144206.504377.patch
Type: text/x-patch
Size: 4292 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230311/cf49c0b4/attachment.bin>