steakhal created this revision.
steakhal added reviewers: aaron.ballman, njames93, LegalizeAdulthood,
whisperity.
Herald added subscribers: carlosgalvezp, martong, rnkovacs, kbarton, xazax.hun,
nemanjai.
Herald added a project: All.
steakhal requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
The `cppcoreguidelines-virtual-class-destructor` supposed to enforce
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c35-a-base-class-destructor-should-be-either-public-and-virtual-or-protected-and-non-virtual
Quote:
> A **base** class destructor should be either public and virtual, or
> protected and non-virtual
[emphasis mine]
However, this check still rules the following case:
class MostDerived final : public Base {
public:
MostDerived() = default;
~MostDerived() = default;
void func() final;
};
Even though `MostDerived` class is marked `final`, thus it should not be
considered as a **base** class. Consequently, the rule is satisfied, yet
the check still flags this code.
In this patch, I'm proposing to ignore `final` classes since they cannot
be //base// classes.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D126891
Files:
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
Index:
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===================================================================
---
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -320,3 +320,22 @@
#undef XMACRO
#undef CONCAT
} // namespace macro_tests
+
+namespace FinalClassCannotBeBaseClass {
+class Base {
+public:
+ Base() = default;
+ virtual void func() = 0;
+
+protected:
+ ~Base() = default;
+};
+
+// no-warning: 'MostDerived' cannot be a base class, since it's marked 'final'.
+class MostDerived final : public Base {
+public:
+ MostDerived() = default;
+ ~MostDerived() = default;
+ void func() final;
+};
+} // namespace FinalClassCannotBeBaseClass
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -165,6 +165,12 @@
Fixed an issue when there was already an initializer in the constructor and
the check would try to create another initializer for the same member.
+- Fixed a false positive in :doc:`cppcoreguidelines-virtual-class-destructor
+ <clang-tidy/checks/cppcoreguidelines-virtual-class-destructor>` involving
+ `final` classes. The check will not diagnose `final` marked classes, since
+ those cannot be used as base classes, consequently they can not violate the
+ rule.
+
- Fixed a crash in :doc:`llvmlibc-callee-namespace
<clang-tidy/checks/llvmlibc-callee-namespace>` when executing for C++ code
that contain calls to advanced constructs, e.g. overloaded operators.
Index:
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===================================================================
---
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -41,6 +41,7 @@
Finder->addMatcher(
cxxRecordDecl(
anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
+ unless(isFinal()),
unless(hasPublicVirtualOrProtectedNonVirtualDestructor()))
.bind("ProblematicClassOrStruct"),
this);
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -320,3 +320,22 @@
#undef XMACRO
#undef CONCAT
} // namespace macro_tests
+
+namespace FinalClassCannotBeBaseClass {
+class Base {
+public:
+ Base() = default;
+ virtual void func() = 0;
+
+protected:
+ ~Base() = default;
+};
+
+// no-warning: 'MostDerived' cannot be a base class, since it's marked 'final'.
+class MostDerived final : public Base {
+public:
+ MostDerived() = default;
+ ~MostDerived() = default;
+ void func() final;
+};
+} // namespace FinalClassCannotBeBaseClass
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -165,6 +165,12 @@
Fixed an issue when there was already an initializer in the constructor and
the check would try to create another initializer for the same member.
+- Fixed a false positive in :doc:`cppcoreguidelines-virtual-class-destructor
+ <clang-tidy/checks/cppcoreguidelines-virtual-class-destructor>` involving
+ `final` classes. The check will not diagnose `final` marked classes, since
+ those cannot be used as base classes, consequently they can not violate the
+ rule.
+
- Fixed a crash in :doc:`llvmlibc-callee-namespace
<clang-tidy/checks/llvmlibc-callee-namespace>` when executing for C++ code
that contain calls to advanced constructs, e.g. overloaded operators.
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -41,6 +41,7 @@
Finder->addMatcher(
cxxRecordDecl(
anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
+ unless(isFinal()),
unless(hasPublicVirtualOrProtectedNonVirtualDestructor()))
.bind("ProblematicClassOrStruct"),
this);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
