Skip to content

Commit

Permalink
[clang-tidy] Fix false-positive in cppcoreguidelines-slicing
Browse files Browse the repository at this point in the history
When warning would be emitted in constructor for virtual base class
initialization.

Fixes: #31187

Reviewed By: carlosgalvezp

Differential Revision: https://reviews.llvm.org/D144206
  • Loading branch information
PiotrZSL committed Mar 11, 2023
1 parent 9b8d944 commit f1e2469
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 9 deletions.
19 changes: 11 additions & 8 deletions clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp
Expand Up @@ -40,30 +40,33 @@ void SlicingCheck::registerMatchers(MatchFinder *Finder) {
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.
Expand Down
6 changes: 5 additions & 1 deletion clang-tools-extra/docs/ReleaseNotes.rst
Expand Up @@ -199,11 +199,15 @@ Changes in existing checks
<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
^^^^^^^^^^^^^^

Expand Down
Expand Up @@ -98,3 +98,25 @@ void negatives() {
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) {}
};

}

0 comments on commit f1e2469

Please sign in to comment.