https://github.com/localspook created https://github.com/llvm/llvm-project/pull/171059
I've structured this PR as a series of independent commits, so it's probably easiest to review commit-by-commit. There's some subjectivity involved with refactoring, so if you think any of the changes is a downgrade, I'm happy to discuss it. >From c53880940787c9c2f81b0466f82e41831b87b407 Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 01:38:48 -0800 Subject: [PATCH 01/18] Inline `addNodeToInterfaceMap` --- .../clang-tidy/fuchsia/MultipleInheritanceCheck.cpp | 11 ++--------- .../clang-tidy/fuchsia/MultipleInheritanceCheck.h | 1 - 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index df40d166bd404..45fea99ece90b 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -23,13 +23,6 @@ AST_MATCHER(CXXRecordDecl, hasBases) { } } // namespace -// Adds a node to the interface map, if it was not present in the map -// previously. -void MultipleInheritanceCheck::addNodeToInterfaceMap(const CXXRecordDecl *Node, - bool IsInterface) { - InterfaceMap.try_emplace(Node, IsInterface); -} - // Returns "true" if the boolean "isInterface" has been set to the // interface status of the current Node. Return "false" if the // interface status for the current node is not yet known. @@ -69,13 +62,13 @@ bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { continue; assert(Base->isCompleteDefinition()); if (!isInterface(Base)) { - addNodeToInterfaceMap(Node, false); + InterfaceMap.try_emplace(Node, false); return false; } } const bool CurrentClassIsInterface = isCurrentClassInterface(Node); - addNodeToInterfaceMap(Node, CurrentClassIsInterface); + InterfaceMap.try_emplace(Node, CurrentClassIsInterface); return CurrentClassIsInterface; } diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h index b9055eb58a300..a333b3c67882e 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h @@ -30,7 +30,6 @@ class MultipleInheritanceCheck : public ClangTidyCheck { void onEndOfTranslationUnit() override { InterfaceMap.clear(); } private: - void addNodeToInterfaceMap(const CXXRecordDecl *Node, bool IsInterface); bool getInterfaceStatus(const CXXRecordDecl *Node, bool &IsInterface) const; bool isCurrentClassInterface(const CXXRecordDecl *Node) const; bool isInterface(const CXXRecordDecl *Node); >From 32cbb6efa3331d055faffefc354c9d81fc2d967f Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 01:41:21 -0800 Subject: [PATCH 02/18] Inline `getInterfaceStatus` At the call site, it was just as much code to call this function as it is to inline it. This also avoids an uninitialized variable. --- .../fuchsia/MultipleInheritanceCheck.cpp | 18 +++--------------- .../fuchsia/MultipleInheritanceCheck.h | 1 - 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index 45fea99ece90b..71dbf6acf906f 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -23,18 +23,6 @@ AST_MATCHER(CXXRecordDecl, hasBases) { } } // namespace -// Returns "true" if the boolean "isInterface" has been set to the -// interface status of the current Node. Return "false" if the -// interface status for the current node is not yet known. -bool MultipleInheritanceCheck::getInterfaceStatus(const CXXRecordDecl *Node, - bool &IsInterface) const { - auto Pair = InterfaceMap.find(Node); - if (Pair == InterfaceMap.end()) - return false; - IsInterface = Pair->second; - return true; -} - bool MultipleInheritanceCheck::isCurrentClassInterface( const CXXRecordDecl *Node) const { // Interfaces should have no fields. @@ -49,9 +37,9 @@ bool MultipleInheritanceCheck::isCurrentClassInterface( bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { // Short circuit the lookup if we have analyzed this record before. - bool PreviousIsInterfaceResult = false; - if (getInterfaceStatus(Node, PreviousIsInterfaceResult)) - return PreviousIsInterfaceResult; + const auto CachedValue = InterfaceMap.find(Node); + if (CachedValue != InterfaceMap.end()) + return CachedValue->second; // To be an interface, all base classes must be interfaces as well. for (const auto &I : Node->bases()) { diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h index a333b3c67882e..feb86da4309fb 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h @@ -30,7 +30,6 @@ class MultipleInheritanceCheck : public ClangTidyCheck { void onEndOfTranslationUnit() override { InterfaceMap.clear(); } private: - bool getInterfaceStatus(const CXXRecordDecl *Node, bool &IsInterface) const; bool isCurrentClassInterface(const CXXRecordDecl *Node) const; bool isInterface(const CXXRecordDecl *Node); >From 91019fc7102a923e4ba114ee65dada5f3949c2cf Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 01:42:01 -0800 Subject: [PATCH 03/18] Prefer preincrement --- .../clang-tidy/fuchsia/MultipleInheritanceCheck.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index 71dbf6acf906f..01caa501e3221 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -79,7 +79,7 @@ void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) { continue; assert(Base->isCompleteDefinition()); if (!isInterface(Base)) - NumConcrete++; + ++NumConcrete; } // Check virtual bases to see if there is more than one concrete @@ -90,7 +90,7 @@ void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) { continue; assert(Base->isCompleteDefinition()); if (!isInterface(Base)) - NumConcrete++; + ++NumConcrete; } if (NumConcrete > 1) { >From 1e50d9c70ec47d105fc89354401acf2861bb98f6 Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 01:44:00 -0800 Subject: [PATCH 04/18] Remove unnecessary check for if node was matched --- .../fuchsia/MultipleInheritanceCheck.cpp | 55 +++++++++---------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index 01caa501e3221..fd8e6a655b359 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -67,36 +67,35 @@ void MultipleInheritanceCheck::registerMatchers(MatchFinder *Finder) { } void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) { - if (const auto *D = Result.Nodes.getNodeAs<CXXRecordDecl>("decl")) { - // Check against map to see if the class inherits from multiple - // concrete classes - unsigned NumConcrete = 0; - for (const auto &I : D->bases()) { - if (I.isVirtual()) - continue; - const auto *Base = I.getType()->getAsCXXRecordDecl(); - if (!Base) - continue; - assert(Base->isCompleteDefinition()); - if (!isInterface(Base)) - ++NumConcrete; - } + const auto &D = *Result.Nodes.getNodeAs<CXXRecordDecl>("decl"); + // Check against map to see if the class inherits from multiple + // concrete classes + unsigned NumConcrete = 0; + for (const auto &I : D.bases()) { + if (I.isVirtual()) + continue; + const auto *Base = I.getType()->getAsCXXRecordDecl(); + if (!Base) + continue; + assert(Base->isCompleteDefinition()); + if (!isInterface(Base)) + ++NumConcrete; + } - // Check virtual bases to see if there is more than one concrete - // non-virtual base. - for (const auto &V : D->vbases()) { - const auto *Base = V.getType()->getAsCXXRecordDecl(); - if (!Base) - continue; - assert(Base->isCompleteDefinition()); - if (!isInterface(Base)) - ++NumConcrete; - } + // Check virtual bases to see if there is more than one concrete + // non-virtual base. + for (const auto &V : D.vbases()) { + const auto *Base = V.getType()->getAsCXXRecordDecl(); + if (!Base) + continue; + assert(Base->isCompleteDefinition()); + if (!isInterface(Base)) + ++NumConcrete; + } - if (NumConcrete > 1) { - diag(D->getBeginLoc(), "inheriting multiple classes that aren't " - "pure virtual is discouraged"); - } + if (NumConcrete > 1) { + diag(D.getBeginLoc(), "inheriting multiple classes that aren't " + "pure virtual is discouraged"); } } >From d71221a16632660192207570ff348c66d214b430 Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 01:46:22 -0800 Subject: [PATCH 05/18] Deduplication `try_emplace` calls --- .../fuchsia/MultipleInheritanceCheck.cpp | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index fd8e6a655b359..507ecc6973a24 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -41,21 +41,20 @@ bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { if (CachedValue != InterfaceMap.end()) return CachedValue->second; - // To be an interface, all base classes must be interfaces as well. - for (const auto &I : Node->bases()) { - if (I.isVirtual()) - continue; - const auto *Base = I.getType()->getAsCXXRecordDecl(); - if (!Base) - continue; - assert(Base->isCompleteDefinition()); - if (!isInterface(Base)) { - InterfaceMap.try_emplace(Node, false); - return false; + const bool CurrentClassIsInterface = [&] { + // To be an interface, all base classes must be interfaces as well. + for (const auto &I : Node->bases()) { + if (I.isVirtual()) + continue; + const auto *Base = I.getType()->getAsCXXRecordDecl(); + if (!Base) + continue; + assert(Base->isCompleteDefinition()); + if (!isInterface(Base)) + return false; } - } - - const bool CurrentClassIsInterface = isCurrentClassInterface(Node); + return isCurrentClassInterface(Node); + }(); InterfaceMap.try_emplace(Node, CurrentClassIsInterface); return CurrentClassIsInterface; } >From 333cb8087fc1c71953b79a72b24f9921b3e5d9c8 Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 01:47:47 -0800 Subject: [PATCH 06/18] Inline `isCurrentClassInterface` Now all the interface criteria is in one place. --- .../fuchsia/MultipleInheritanceCheck.cpp | 22 ++++++++----------- .../fuchsia/MultipleInheritanceCheck.h | 1 - 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index 507ecc6973a24..cbc76f81356cb 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -23,18 +23,6 @@ AST_MATCHER(CXXRecordDecl, hasBases) { } } // namespace -bool MultipleInheritanceCheck::isCurrentClassInterface( - const CXXRecordDecl *Node) const { - // Interfaces should have no fields. - if (!Node->field_empty()) - return false; - - // Interfaces should have exclusively pure methods. - return llvm::none_of(Node->methods(), [](const CXXMethodDecl *M) { - return M->isUserProvided() && !M->isPureVirtual() && !M->isStatic(); - }); -} - bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { // Short circuit the lookup if we have analyzed this record before. const auto CachedValue = InterfaceMap.find(Node); @@ -53,7 +41,15 @@ bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { if (!isInterface(Base)) return false; } - return isCurrentClassInterface(Node); + + // Interfaces should have no fields. + if (!Node->field_empty()) + return false; + + // Interfaces should have exclusively pure methods. + return llvm::none_of(Node->methods(), [](const CXXMethodDecl *M) { + return M->isUserProvided() && !M->isPureVirtual() && !M->isStatic(); + }); }(); InterfaceMap.try_emplace(Node, CurrentClassIsInterface); return CurrentClassIsInterface; diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h index feb86da4309fb..699037e8e38e0 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h @@ -30,7 +30,6 @@ class MultipleInheritanceCheck : public ClangTidyCheck { void onEndOfTranslationUnit() override { InterfaceMap.clear(); } private: - bool isCurrentClassInterface(const CXXRecordDecl *Node) const; bool isInterface(const CXXRecordDecl *Node); // Contains the identity of each named CXXRecord as an interface. This is >From a5a0e5ce5bd4a1a0f05d547440e39ef630e15e0f Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 01:50:32 -0800 Subject: [PATCH 07/18] Explicitly spell type where it isn't visible in the initializer --- .../clang-tidy/fuchsia/MultipleInheritanceCheck.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index cbc76f81356cb..4ca56cbb02167 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -31,7 +31,7 @@ bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { const bool CurrentClassIsInterface = [&] { // To be an interface, all base classes must be interfaces as well. - for (const auto &I : Node->bases()) { + for (const CXXBaseSpecifier &I : Node->bases()) { if (I.isVirtual()) continue; const auto *Base = I.getType()->getAsCXXRecordDecl(); @@ -66,7 +66,7 @@ void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) { // Check against map to see if the class inherits from multiple // concrete classes unsigned NumConcrete = 0; - for (const auto &I : D.bases()) { + for (const CXXBaseSpecifier &I : D.bases()) { if (I.isVirtual()) continue; const auto *Base = I.getType()->getAsCXXRecordDecl(); @@ -79,7 +79,7 @@ void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) { // Check virtual bases to see if there is more than one concrete // non-virtual base. - for (const auto &V : D.vbases()) { + for (const CXXBaseSpecifier &V : D.vbases()) { const auto *Base = V.getType()->getAsCXXRecordDecl(); if (!Base) continue; >From 4a4c4dcfb88419bcdf5d8e9352a618c1e7abaccd Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 01:51:21 -0800 Subject: [PATCH 08/18] Deduplicate `isCompleteDefinition` assertions --- .../clang-tidy/fuchsia/MultipleInheritanceCheck.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index 4ca56cbb02167..c371150be6bec 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -24,6 +24,8 @@ AST_MATCHER(CXXRecordDecl, hasBases) { } // namespace bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { + assert(Node->isCompleteDefinition()); + // Short circuit the lookup if we have analyzed this record before. const auto CachedValue = InterfaceMap.find(Node); if (CachedValue != InterfaceMap.end()) @@ -37,7 +39,6 @@ bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { const auto *Base = I.getType()->getAsCXXRecordDecl(); if (!Base) continue; - assert(Base->isCompleteDefinition()); if (!isInterface(Base)) return false; } @@ -72,7 +73,6 @@ void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) { const auto *Base = I.getType()->getAsCXXRecordDecl(); if (!Base) continue; - assert(Base->isCompleteDefinition()); if (!isInterface(Base)) ++NumConcrete; } @@ -83,7 +83,6 @@ void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) { const auto *Base = V.getType()->getAsCXXRecordDecl(); if (!Base) continue; - assert(Base->isCompleteDefinition()); if (!isInterface(Base)) ++NumConcrete; } >From 6e5958e18c2ced9b1ba5ce540f251281d907491d Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 01:53:16 -0800 Subject: [PATCH 09/18] Remove unhelpful comment --- .../clang-tidy/fuchsia/MultipleInheritanceCheck.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index c371150be6bec..752e58d05f2d6 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -57,7 +57,6 @@ bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { } void MultipleInheritanceCheck::registerMatchers(MatchFinder *Finder) { - // Match declarations which have bases. Finder->addMatcher(cxxRecordDecl(hasBases(), isDefinition()).bind("decl"), this); } >From e6fd89bc6190f8886ba11c553a6da8af9ba34fb2 Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 01:57:20 -0800 Subject: [PATCH 10/18] Correct stale (?) comment --- .../clang-tidy/fuchsia/MultipleInheritanceCheck.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index 752e58d05f2d6..63efaa27d27d2 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -63,8 +63,7 @@ void MultipleInheritanceCheck::registerMatchers(MatchFinder *Finder) { void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) { const auto &D = *Result.Nodes.getNodeAs<CXXRecordDecl>("decl"); - // Check against map to see if the class inherits from multiple - // concrete classes + // Check to see if the class inherits from multiple concrete classes. unsigned NumConcrete = 0; for (const CXXBaseSpecifier &I : D.bases()) { if (I.isVirtual()) >From b4f7436fe0751502caf0389f1c1a4ffee4c76352 Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 02:05:30 -0800 Subject: [PATCH 11/18] Deduplicate null checks --- .../fuchsia/MultipleInheritanceCheck.cpp | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index 63efaa27d27d2..3972a0be3a2ef 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -24,6 +24,9 @@ AST_MATCHER(CXXRecordDecl, hasBases) { } // namespace bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { + if (!Node) + return false; + assert(Node->isCompleteDefinition()); // Short circuit the lookup if we have analyzed this record before. @@ -36,10 +39,7 @@ bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { for (const CXXBaseSpecifier &I : Node->bases()) { if (I.isVirtual()) continue; - const auto *Base = I.getType()->getAsCXXRecordDecl(); - if (!Base) - continue; - if (!isInterface(Base)) + if (!isInterface(I.getType()->getAsCXXRecordDecl())) return false; } @@ -68,20 +68,14 @@ void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) { for (const CXXBaseSpecifier &I : D.bases()) { if (I.isVirtual()) continue; - const auto *Base = I.getType()->getAsCXXRecordDecl(); - if (!Base) - continue; - if (!isInterface(Base)) + if (!isInterface(I.getType()->getAsCXXRecordDecl())) ++NumConcrete; } // Check virtual bases to see if there is more than one concrete // non-virtual base. for (const CXXBaseSpecifier &V : D.vbases()) { - const auto *Base = V.getType()->getAsCXXRecordDecl(); - if (!Base) - continue; - if (!isInterface(Base)) + if (!isInterface(V.getType()->getAsCXXRecordDecl())) ++NumConcrete; } >From 7f8e3106f1cf58feae88c6e918c61c33290f7098 Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 02:07:21 -0800 Subject: [PATCH 12/18] Merge a couple `if` statements --- .../clang-tidy/fuchsia/MultipleInheritanceCheck.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index 3972a0be3a2ef..e22040e14d27e 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -37,9 +37,7 @@ bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { const bool CurrentClassIsInterface = [&] { // To be an interface, all base classes must be interfaces as well. for (const CXXBaseSpecifier &I : Node->bases()) { - if (I.isVirtual()) - continue; - if (!isInterface(I.getType()->getAsCXXRecordDecl())) + if (!I.isVirtual() && !isInterface(I.getType()->getAsCXXRecordDecl())) return false; } @@ -66,9 +64,7 @@ void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) { // Check to see if the class inherits from multiple concrete classes. unsigned NumConcrete = 0; for (const CXXBaseSpecifier &I : D.bases()) { - if (I.isVirtual()) - continue; - if (!isInterface(I.getType()->getAsCXXRecordDecl())) + if (!I.isVirtual() && !isInterface(I.getType()->getAsCXXRecordDecl())) ++NumConcrete; } >From 514d85357e988bfec7cde59b349ecacd4181f0d4 Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 02:26:18 -0800 Subject: [PATCH 13/18] pure and pure virtual are very different things --- .../clang-tidy/fuchsia/MultipleInheritanceCheck.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index e22040e14d27e..7687176d33a8f 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -45,7 +45,7 @@ bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { if (!Node->field_empty()) return false; - // Interfaces should have exclusively pure methods. + // Interfaces should have exclusively pure virtual methods. return llvm::none_of(Node->methods(), [](const CXXMethodDecl *M) { return M->isUserProvided() && !M->isPureVirtual() && !M->isStatic(); }); >From 6ae8fcdc4a6be896283908bf1576e0dbab08af9f Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 13:55:02 -0800 Subject: [PATCH 14/18] Make isInterface take a CXXBaseSpecifier --- .../clang-tidy/fuchsia/MultipleInheritanceCheck.cpp | 9 +++++---- .../clang-tidy/fuchsia/MultipleInheritanceCheck.h | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index 7687176d33a8f..a5fba372a9a16 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -23,7 +23,8 @@ AST_MATCHER(CXXRecordDecl, hasBases) { } } // namespace -bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { +bool MultipleInheritanceCheck::isInterface(const CXXBaseSpecifier &Base) { + const CXXRecordDecl *const Node = Base.getType()->getAsCXXRecordDecl(); if (!Node) return false; @@ -37,7 +38,7 @@ bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) { const bool CurrentClassIsInterface = [&] { // To be an interface, all base classes must be interfaces as well. for (const CXXBaseSpecifier &I : Node->bases()) { - if (!I.isVirtual() && !isInterface(I.getType()->getAsCXXRecordDecl())) + if (!I.isVirtual() && !isInterface(I)) return false; } @@ -64,14 +65,14 @@ void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) { // Check to see if the class inherits from multiple concrete classes. unsigned NumConcrete = 0; for (const CXXBaseSpecifier &I : D.bases()) { - if (!I.isVirtual() && !isInterface(I.getType()->getAsCXXRecordDecl())) + if (!I.isVirtual() && !isInterface(I)) ++NumConcrete; } // Check virtual bases to see if there is more than one concrete // non-virtual base. for (const CXXBaseSpecifier &V : D.vbases()) { - if (!isInterface(V.getType()->getAsCXXRecordDecl())) + if (!isInterface(V)) ++NumConcrete; } diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h index 699037e8e38e0..eb5c94241996e 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h @@ -30,7 +30,7 @@ class MultipleInheritanceCheck : public ClangTidyCheck { void onEndOfTranslationUnit() override { InterfaceMap.clear(); } private: - bool isInterface(const CXXRecordDecl *Node); + bool isInterface(const CXXBaseSpecifier& Base); // Contains the identity of each named CXXRecord as an interface. This is // used to memoize lookup speeds and improve performance from O(N^2) to O(N), >From 8c24a9bec1b355fc69f8d41bf31dfff93f41f390 Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 14:02:14 -0800 Subject: [PATCH 15/18] Use count_if algorithm --- .../fuchsia/MultipleInheritanceCheck.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index a5fba372a9a16..c58a9adc6d386 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -63,18 +63,15 @@ void MultipleInheritanceCheck::registerMatchers(MatchFinder *Finder) { void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) { const auto &D = *Result.Nodes.getNodeAs<CXXRecordDecl>("decl"); // Check to see if the class inherits from multiple concrete classes. - unsigned NumConcrete = 0; - for (const CXXBaseSpecifier &I : D.bases()) { - if (!I.isVirtual() && !isInterface(I)) - ++NumConcrete; - } + unsigned NumConcrete = + llvm::count_if(D.bases(), [&](const CXXBaseSpecifier &I) { + return !I.isVirtual() && !isInterface(I); + }); // Check virtual bases to see if there is more than one concrete // non-virtual base. - for (const CXXBaseSpecifier &V : D.vbases()) { - if (!isInterface(V)) - ++NumConcrete; - } + NumConcrete += llvm::count_if( + D.vbases(), [&](const CXXBaseSpecifier &V) { return !isInterface(V); }); if (NumConcrete > 1) { diag(D.getBeginLoc(), "inheriting multiple classes that aren't " >From 0bbf692a8bb6edf47a5b947aaf9cae0e2c3def1c Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 14:03:41 -0800 Subject: [PATCH 16/18] Use any_of algorithm --- .../clang-tidy/fuchsia/MultipleInheritanceCheck.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index c58a9adc6d386..ccefadfbfdd49 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -37,10 +37,10 @@ bool MultipleInheritanceCheck::isInterface(const CXXBaseSpecifier &Base) { const bool CurrentClassIsInterface = [&] { // To be an interface, all base classes must be interfaces as well. - for (const CXXBaseSpecifier &I : Node->bases()) { - if (!I.isVirtual() && !isInterface(I)) - return false; - } + if (llvm::any_of(Node->bases(), [&](const CXXBaseSpecifier &I) { + return !I.isVirtual() && !isInterface(I); + })) + return false; // Interfaces should have no fields. if (!Node->field_empty()) >From 332efab2bea824e61e314a12437879af982c3ef2 Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 14:26:54 -0800 Subject: [PATCH 17/18] Reduce lambda down to conditional expression --- .../fuchsia/MultipleInheritanceCheck.cpp | 28 +++++++++---------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index ccefadfbfdd49..efbe339c955ed 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -35,22 +35,20 @@ bool MultipleInheritanceCheck::isInterface(const CXXBaseSpecifier &Base) { if (CachedValue != InterfaceMap.end()) return CachedValue->second; - const bool CurrentClassIsInterface = [&] { - // To be an interface, all base classes must be interfaces as well. - if (llvm::any_of(Node->bases(), [&](const CXXBaseSpecifier &I) { - return !I.isVirtual() && !isInterface(I); - })) - return false; - - // Interfaces should have no fields. - if (!Node->field_empty()) - return false; + // To be an interface, a class must have... + const bool CurrentClassIsInterface = + // ...no bases that aren't interfaces... + llvm::none_of(Node->bases(), + [&](const CXXBaseSpecifier &I) { + return !I.isVirtual() && !isInterface(I); + }) && + // ...no fields, and... + Node->field_empty() && + // ...no methods that aren't pure virtual. + llvm::none_of(Node->methods(), [](const CXXMethodDecl *M) { + return M->isUserProvided() && !M->isPureVirtual() && !M->isStatic(); + }); - // Interfaces should have exclusively pure virtual methods. - return llvm::none_of(Node->methods(), [](const CXXMethodDecl *M) { - return M->isUserProvided() && !M->isPureVirtual() && !M->isStatic(); - }); - }(); InterfaceMap.try_emplace(Node, CurrentClassIsInterface); return CurrentClassIsInterface; } >From 9284d780a0ad58286f519932aac803f437dd4530 Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 7 Dec 2025 14:28:02 -0800 Subject: [PATCH 18/18] Shorten matcher --- .../clang-tidy/fuchsia/MultipleInheritanceCheck.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp index efbe339c955ed..aa5f4f847c762 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp @@ -17,9 +17,7 @@ namespace clang::tidy::fuchsia { namespace { AST_MATCHER(CXXRecordDecl, hasBases) { - if (Node.hasDefinition()) - return Node.getNumBases() > 0; - return false; + return Node.hasDefinition() && Node.getNumBases() > 0; } } // namespace _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
