[clang] [NFC][analyzer] Simplify ownership of checker objects (PR #128887)
Donát Nagy via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 27 05:09:53 PST 2025
https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/128887
>From 76f8417b8b46e7d036d98fa92890469654158e20 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Wed, 26 Feb 2025 15:41:46 +0100
Subject: [PATCH 1/5] [NFC][analyzer] Simplify ownership of checker objects
Previously checker objects were created by raw `new` calls, which
necessitated managing and calling their destructors explicitly. This
commit refactors this convoluted logic by introducing `unique_ptr`s that
to manage the ownership of these objects automatically.
This change can be thought of as stand-alone code quality improvement;
but I also have a secondary motivation that I'm planning further changes
in the checker registration/initialization process (to formalize our
tradition of multi-part checker) and this commit "prepares the ground"
for those changes.
---
.../StaticAnalyzer/Core/CheckerManager.h | 40 +++++++++----------
.../Frontend/CreateCheckerManager.cpp | 5 ++-
2 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index de40b96614dbc..b48a75630fe9b 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -185,13 +185,11 @@ class CheckerManager {
StringRef OptionName,
StringRef ExpectedValueDesc) const;
- using CheckerRef = CheckerBase *;
using CheckerTag = const void *;
- using CheckerDtor = CheckerFn<void ()>;
-//===----------------------------------------------------------------------===//
-// Checker registration.
-//===----------------------------------------------------------------------===//
+ //===----------------------------------------------------------------------===//
+ // Checker registration.
+ //===----------------------------------------------------------------------===//
/// Used to register checkers.
/// All arguments are automatically passed through to the checker
@@ -201,24 +199,24 @@ class CheckerManager {
template <typename CHECKER, typename... AT>
CHECKER *registerChecker(AT &&... Args) {
CheckerTag tag = getTag<CHECKER>();
- CheckerRef &ref = CheckerTags[tag];
- assert(!ref && "Checker already registered, use getChecker!");
-
- CHECKER *checker = new CHECKER(std::forward<AT>(Args)...);
- checker->Name = CurrentCheckerName;
- CheckerDtors.push_back(CheckerDtor(checker, destruct<CHECKER>));
- CHECKER::_register(checker, *this);
- ref = checker;
- return checker;
+ std::unique_ptr<CheckerBase> &Ref = CheckerTags[tag];
+ assert(!Ref && "Checker already registered, use getChecker!");
+
+ std::unique_ptr<CHECKER> Checker =
+ std::make_unique<CHECKER>(std::forward<AT>(Args)...);
+ Checker->Name = CurrentCheckerName;
+ CHECKER::_register(Checker.get(), *this);
+ Ref = std::move(Checker);
+ return static_cast<CHECKER *>(Ref.get());
}
template <typename CHECKER>
CHECKER *getChecker() {
- CheckerTag tag = getTag<CHECKER>();
- assert(CheckerTags.count(tag) != 0 &&
- "Requested checker is not registered! Maybe you should add it as a "
- "dependency in Checkers.td?");
- return static_cast<CHECKER *>(CheckerTags[tag]);
+ CheckerTag Tag = getTag<CHECKER>();
+ std::unique_ptr<CheckerBase> &Ref = CheckerTags[Tag];
+ assert(Ref && "Requested checker is not registered! Maybe you should add it"
+ " as a dependency in Checkers.td?");
+ return static_cast<CHECKER *>(Ref.get());
}
template <typename CHECKER> bool isRegisteredChecker() {
@@ -622,9 +620,7 @@ class CheckerManager {
template <typename T>
static void *getTag() { static int tag; return &tag; }
- llvm::DenseMap<CheckerTag, CheckerRef> CheckerTags;
-
- std::vector<CheckerDtor> CheckerDtors;
+ llvm::DenseMap<CheckerTag, std::unique_ptr<CheckerBase>> CheckerTags;
struct DeclCheckerInfo {
CheckDeclFunc CheckFn;
diff --git a/clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp b/clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
index f60221ad7587e..35dd255fce898 100644
--- a/clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
+++ b/clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
@@ -10,6 +10,7 @@
//
//===----------------------------------------------------------------------===//
+#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
#include <memory>
@@ -41,8 +42,8 @@ CheckerManager::CheckerManager(AnalyzerOptions &AOptions,
}
CheckerManager::~CheckerManager() {
- for (const auto &CheckerDtor : CheckerDtors)
- CheckerDtor();
+ // This is declared here to ensure that the destructors of `CheckerBase` and
+ // `CheckerRegistryData` are available.
}
} // namespace ento
>From 38774503663f74ca1c8b36d7ed1b55c10f9edd5f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 27 Feb 2025 14:00:10 +0100
Subject: [PATCH 2/5] Set destructor to '= default' instead of using empty body
---
clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp b/clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
index 35dd255fce898..4bfc91369e7ab 100644
--- a/clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
+++ b/clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
@@ -41,10 +41,9 @@ CheckerManager::CheckerManager(AnalyzerOptions &AOptions,
Registry.initializeRegistry(*this);
}
-CheckerManager::~CheckerManager() {
- // This is declared here to ensure that the destructors of `CheckerBase` and
- // `CheckerRegistryData` are available.
-}
+// This is declared here to ensure that the destructors of `CheckerBase` and
+// `CheckerRegistryData` are available.
+CheckerManager::~CheckerManager() = default;
} // namespace ento
} // namespace clang
>From 9f4a8a8d3bf50d29e7bd67f5a9b4142bbe068abc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 27 Feb 2025 14:01:06 +0100
Subject: [PATCH 3/5] Remove helper method 'destruct()' which became unused
---
clang/include/clang/StaticAnalyzer/Core/CheckerManager.h | 3 ---
1 file changed, 3 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index b48a75630fe9b..93cfbf3c9fc36 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -614,9 +614,6 @@ class CheckerManager {
//===----------------------------------------------------------------------===//
private:
- template <typename CHECKER>
- static void destruct(void *obj) { delete static_cast<CHECKER *>(obj); }
-
template <typename T>
static void *getTag() { static int tag; return &tag; }
>From 9bbd113365f2fe989fe30e378eab94c3bf297d36 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 27 Feb 2025 14:03:46 +0100
Subject: [PATCH 4/5] Indent heading comments to satisfy git-clang-format
---
.../StaticAnalyzer/Core/CheckerManager.h | 22 +++++++++----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index 93cfbf3c9fc36..23fc89dd0a51b 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -460,9 +460,9 @@ class CheckerManager {
unsigned int Space = 0,
bool IsDot = false) const;
- //===----------------------------------------------------------------------===//
+ //===--------------------------------------------------------------------===//
// Internal registration functions for AST traversing.
- //===----------------------------------------------------------------------===//
+ //===--------------------------------------------------------------------===//
// Functions used by the registration mechanism, checkers should not touch
// these directly.
@@ -476,9 +476,9 @@ class CheckerManager {
void _registerForBody(CheckDeclFunc checkfn);
-//===----------------------------------------------------------------------===//
-// Internal registration functions for path-sensitive checking.
-//===----------------------------------------------------------------------===//
+ //===--------------------------------------------------------------------===//
+ // Internal registration functions for path-sensitive checking.
+ //===--------------------------------------------------------------------===//
using CheckStmtFunc = CheckerFn<void (const Stmt *, CheckerContext &)>;
@@ -580,9 +580,9 @@ class CheckerManager {
void _registerForEndOfTranslationUnit(CheckEndOfTranslationUnit checkfn);
-//===----------------------------------------------------------------------===//
-// Internal registration functions for events.
-//===----------------------------------------------------------------------===//
+ //===--------------------------------------------------------------------===//
+ // Internal registration functions for events.
+ //===--------------------------------------------------------------------===//
using EventTag = void *;
using CheckEventFunc = CheckerFn<void (const void *event)>;
@@ -609,9 +609,9 @@ class CheckerManager {
Checker(&event);
}
-//===----------------------------------------------------------------------===//
-// Implementation details.
-//===----------------------------------------------------------------------===//
+ //===--------------------------------------------------------------------===//
+ // Implementation details.
+ //===--------------------------------------------------------------------===//
private:
template <typename T>
>From 514b3b0ec8884ea1902def8aa993edba4c0addb8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 27 Feb 2025 14:09:31 +0100
Subject: [PATCH 5/5] Reduce another heading comment to 80 columns
---
clang/include/clang/StaticAnalyzer/Core/CheckerManager.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index 23fc89dd0a51b..16764db44e1c9 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -187,9 +187,9 @@ class CheckerManager {
using CheckerTag = const void *;
- //===----------------------------------------------------------------------===//
+ //===--------------------------------------------------------------------===//
// Checker registration.
- //===----------------------------------------------------------------------===//
+ //===--------------------------------------------------------------------===//
/// Used to register checkers.
/// All arguments are automatically passed through to the checker
More information about the cfe-commits
mailing list