Make *Util components constructible

Previously (603949f) we made some
changes to `TrimUtil` to return a `std::shared_ptr<void>` containing the
internal variables used in the algorithm.  This allowed us to defer
destruction in `TrimUtil` so that we can skip it with `std::_Exit` later
on - saving 1-2%.

The change did not sit well with me and is not very idiomatic.  This
commit changes both `BuildDirUtil` and `TrimUtil` to be constructible
and have their static methods changed to non-`const` member variables.
We store the variables in a `std::unique_ptr` to defer destruction of
the state until the destructor of the `*Util` - which is skipped when we
call `std::_Exit`.

src/builddirutil.cpp

@@ -32,9 +32,10 @@
 
 namespace trimja {
 
-namespace {
+namespace detail {
 
-struct BuildDirContext {
+class BuildDirContext {
+ public:
   BasicScope fileScope;
 
   BuildDirContext() = default;
@@ -100,18 +101,25 @@ struct BuildDirContext {
   }
 };
 
-}  // namespace
+}  // namespace detail
+
+BuildDirUtil::BuildDirUtil() : m_imp{nullptr} {}
+
+BuildDirUtil::~BuildDirUtil() = default;
 
 std::filesystem::path BuildDirUtil::builddir(
     const std::filesystem::path& ninjaFile,
     const std::string& ninjaFileContents) {
-  BuildDirContext ctx;
+  // Keep our state inside `m_imp` so that we defer cleanup until the destructor
+  // of `BuildDirUtil`. This allows the calling code to skip all destructors
+  // when calling `std::_Exit`.
+  m_imp = std::make_unique<detail::BuildDirContext>();
   {
     const Timer t = CPUProfiler::start(".ninja parse");
-    ctx.parse(ninjaFile, ninjaFileContents);
+    m_imp->parse(ninjaFile, ninjaFileContents);
   }
   std::string builddir;
-  ctx.fileScope.appendValue(builddir, "builddir");
+  m_imp->fileScope.appendValue(builddir, "builddir");
   return std::filesystem::path(ninjaFile).remove_filename() / builddir;
 }
 

src/builddirutil.h

@@ -28,20 +28,37 @@
 
 namespace trimja {
 
+namespace detail {
+class BuildDirContext;
+}  // namespace detail
+
 /**
- * @struct BuildDirUtil
+ * @class BuildDirUtil
  * @brief Utility functions for finding the build directory for a Ninja file.
  */
-struct BuildDirUtil {
+class BuildDirUtil {
+  std::unique_ptr<detail::BuildDirContext> m_imp;
+
+ public:
+  /**
+   * @brief Default constructor for BuildDirUtil.
+   */
+  BuildDirUtil();
+
+  /**
+   * @brief Destructor for BuildDirUtil.
+   */
+  ~BuildDirUtil();
+
   /**
    * @brief Determines the build directory from the given Ninja file and its
    * contents.
    * @param ninjaFile The path to the Ninja file.
    * @param ninjaFileContents The contents of the Ninja file.
    * @return The path to the build directory.
    */
-  static std::filesystem::path builddir(const std::filesystem::path& ninjaFile,
-                                        const std::string& ninjaFileContents);
+  std::filesystem::path builddir(const std::filesystem::path& ninjaFile,
+                                 const std::string& ninjaFileContents);
 };
 
 }  // namespace trimja

src/trimja.m.cpp

@@ -277,7 +277,8 @@ bool instrumentMemory = false;
 
   // If we have `--builddir` then ignore all other flags other than -f
   if (builddir) {
-    std::cout << BuildDirUtil::builddir(ninjaFile, ninjaFileContents).string()
+    BuildDirUtil util;
+    std::cout << util.builddir(ninjaFile, ninjaFileContents).string()
               << std::endl;
     leave(EXIT_SUCCESS);
   }
@@ -316,10 +317,8 @@ bool instrumentMemory = false;
       },
       outputFile);
 
-  // Keep the variables used in `trim` alive so that we can skip destruction
-  // and its overhead when we call `leave`.
-  [[maybe_unused]] const std::shared_ptr<void> state =
-      TrimUtil::trim(output, ninjaFile, ninjaFileContents, affected, explain);
+  TrimUtil util;
+  util.trim(output, ninjaFile, ninjaFileContents, affected, explain);
   output.flush();
 
   if (!expectedFile.has_value()) {

src/trimutil.cpp

@@ -192,7 +192,12 @@ struct RuleCommand {
   RuleCommand(std::string_view name) : name{name} {}
 };
 
-struct BuildContext {
+}  // namespace
+
+namespace detail {
+
+class BuildContext {
+ public:
   // The indexes of the built-in rules within `rules`
   static const std::size_t phonyIndex = 0;
   static const std::size_t defaultIndex = 1;
@@ -647,9 +652,13 @@ struct BuildContext {
   }
 };
 
+}  // namespace detail
+
+namespace {
+
 void parseDepFile(const std::filesystem::path& ninjaDeps,
                   Graph& graph,
-                  BuildContext& ctx) {
+                  detail::BuildContext& ctx) {
   // Later entries may override earlier entries so don't touch the graph until
   // we have parsed the whole file
   std::vector<std::string> paths;
@@ -692,7 +701,7 @@ void parseDepFile(const std::filesystem::path& ninjaDeps,
 
 template <typename GET_HASH>
 void parseLogFile(const std::filesystem::path& ninjaLog,
-                  const BuildContext& ctx,
+                  const detail::BuildContext& ctx,
                   std::vector<bool>& isAffected,
                   GET_HASH&& get_hash,
                   bool explain) {
@@ -725,7 +734,7 @@ void parseLogFile(const std::filesystem::path& ninjaLog,
     }
 
     // buil-in rules don't appear in the build log so skip them
-    if (BuildContext::isBuiltInRule(
+    if (detail::BuildContext::isBuiltInRule(
             ctx.commands[ctx.nodeToCommand[index]].ruleIndex)) {
       continue;
     }
@@ -754,7 +763,7 @@ void parseLogFile(const std::filesystem::path& ninjaLog,
 void markIfChildrenAffected(std::size_t index,
                             std::vector<bool>& seen,
                             std::vector<bool>& isAffected,
-                            const BuildContext& ctx,
+                            const detail::BuildContext& ctx,
                             bool explain) {
   if (seen[index]) {
     return;
@@ -780,7 +789,7 @@ void markIfChildrenAffected(std::size_t index,
   if (it != inIndices.end()) {
     if (explain) {
       // Only mention user-defined rules since built-in rules are always kept
-      if (!BuildContext::isBuiltInRule(
+      if (!detail::BuildContext::isBuiltInRule(
               ctx.commands[ctx.nodeToCommand[index]].ruleIndex)) {
         std::cerr << "Including '" << graph.path(index)
                   << "' as it has the affected input '" << graph.path(*it)
@@ -798,7 +807,7 @@ void ifAffectedMarkAllChildren(std::size_t index,
                                std::vector<bool>& seen,
                                std::vector<bool>& isAffected,
                                std::vector<bool>& needsAllInputs,
-                               const BuildContext& ctx,
+                               const detail::BuildContext& ctx,
                                bool explain) {
   if (seen[index]) {
     return;
@@ -815,7 +824,7 @@ void ifAffectedMarkAllChildren(std::size_t index,
     return;
   }
 
-  if (!BuildContext::isBuiltInRule(
+  if (!detail::BuildContext::isBuiltInRule(
           ctx.commands[ctx.nodeToCommand[index]].ruleIndex)) {
     if (isAffected[index]) {
       needsAllInputs[index] = true;
@@ -844,13 +853,20 @@ void ifAffectedMarkAllChildren(std::size_t index,
 
 }  // namespace
 
-std::shared_ptr<void> TrimUtil::trim(std::ostream& output,
-                                     const std::filesystem::path& ninjaFile,
-                                     const std::string& ninjaFileContents,
-                                     std::istream& affected,
-                                     bool explain) {
-  auto state = std::make_shared<BuildContext>();
-  BuildContext& ctx = *state;
+TrimUtil::TrimUtil() : m_imp{nullptr} {}
+
+TrimUtil::~TrimUtil() = default;
+
+void TrimUtil::trim(std::ostream& output,
+                    const std::filesystem::path& ninjaFile,
+                    const std::string& ninjaFileContents,
+                    std::istream& affected,
+                    bool explain) {
+  // Keep our state inside `m_imp` so that we defer cleanup until the destructor
+  // of `TrimUtil`. This allows the calling code to skip all destructors when
+  // calling `std::_Exit`.
+  m_imp = std::make_unique<detail::BuildContext>();
+  detail::BuildContext& ctx = *m_imp;
 
   // Parse the build file, this needs to be the first thing so we choose the
   // canonical paths in the same way that ninja does
@@ -1062,8 +1078,6 @@ std::shared_ptr<void> TrimUtil::trim(std::ostream& output,
   const Timer writeTimer = CPUProfiler::start("output time");
   std::copy(ctx.parts.begin(), ctx.parts.end(),
             std::ostream_iterator<std::string_view>(output));
-
-  return state;
 }
 
 }  // namespace trimja

src/trimutil.h

@@ -30,11 +30,28 @@
 
 namespace trimja {
 
+namespace detail {
+class BuildContext;
+}  // namespace detail
+
 /**
- * @struct TrimUtil
+ * @class TrimUtil
  * @brief Utility to trim a Ninja build file based on a list of affected files.
  */
-struct TrimUtil {
+class TrimUtil {
+  std::unique_ptr<detail::BuildContext> m_imp;
+
+ public:
+  /**
+   * @brief Default constructor for TrimUtil.
+   */
+  TrimUtil();
+
+  /**
+   * @brief Destructor for TrimUtil.
+   */
+  ~TrimUtil();
+
   /**
    * @brief Trims the given Ninja build file based on the affected files.
    *
@@ -43,14 +60,12 @@ struct TrimUtil {
    * @param ninjaFileContents The contents of the original Ninja build file.
    * @param affected The input stream containing the list of affected files.
    * @param explain If true, prints to stderr why each build command was kept.
-   * @return A shared pointer containing the internal variables used in the
-   * algorithm to defer or avoid destruction.
    */
-  static std::shared_ptr<void> trim(std::ostream& output,
-                                    const std::filesystem::path& ninjaFile,
-                                    const std::string& ninjaFileContents,
-                                    std::istream& affected,
-                                    bool explain);
+  void trim(std::ostream& output,
+            const std::filesystem::path& ninjaFile,
+            const std::string& ninjaFileContents,
+            std::istream& affected,
+            bool explain);
 };
 
 }  // namespace trimja