[NFC][analyzer] Simplify ownership of checker objects

[NagyDonat]

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_ptrs 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.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Donát Nagy (NagyDonat)

Changes

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_ptrs 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.

---

Full diff: https://github.com/llvm/llvm-project/pull/128887.diff

2 Files Affected:

• (modified) clang/include/clang/StaticAnalyzer/Core/CheckerManager.h (+18-22)
• (modified) clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp (+3-2)

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

[NagyDonat]

This indentation was added automatically by git-clang-format 🙄

There are several similar section markers in this source file -- should I indent all of them for consistency, or is there a reasonable way to silence the automatic formatting?

[Xazax-hun]

You can do:

// clang-format off
    void    unformatted_code  ;
// clang-format on

But I don't think that is worth it here. I am OK with just changing this code to make clang-format happy.

[steakhal]

We could as well just drop this comment.

[NagyDonat]

I think I'll indent this comment and the other analogous headings to satisfy git-clang-format. I don't want to remove them because I feel that visible headers like this are very useful when I want to quickly understand the architecture of a large cumbersome class.

[Xazax-hun]

You can do:

// clang-format off
    void    unformatted_code  ;
// clang-format on

But I don't think that is worth it here. I am OK with just changing this code to make clang-format happy.