Merge pull request #2451 from digit-google/explanations-class

Remove global `explanations_` variable

CMakeLists.txt

@@ -260,6 +260,7 @@ if(BUILD_TESTING)
     src/disk_interface_test.cc
     src/dyndep_parser_test.cc
     src/edit_distance_test.cc
+    src/explanations_test.cc
     src/graph_test.cc
     src/json_test.cc
     src/lexer_test.cc

configure.py

@@ -638,6 +638,7 @@ def has_re2c() -> bool:
         'disk_interface_test',
         'dyndep_parser_test',
         'edit_distance_test',
+        'explanations_test',
         'graph_test',
         'json_test',
         'lexer_test',

src/build.cc

@@ -34,6 +34,7 @@
 #include "depfile_parser.h"
 #include "deps_log.h"
 #include "disk_interface.h"
+#include "explanations.h"
 #include "graph.h"
 #include "metrics.h"
 #include "state.h"
@@ -669,22 +670,24 @@ bool RealCommandRunner::WaitForCommand(Result* result) {
   return true;
 }
 
-Builder::Builder(State* state, const BuildConfig& config,
-                 BuildLog* build_log, DepsLog* deps_log,
-                 DiskInterface* disk_interface, Status *status,
-                 int64_t start_time_millis)
+Builder::Builder(State* state, const BuildConfig& config, BuildLog* build_log,
+                 DepsLog* deps_log, DiskInterface* disk_interface,
+                 Status* status, int64_t start_time_millis)
     : state_(state), config_(config), plan_(this), status_(status),
       start_time_millis_(start_time_millis), disk_interface_(disk_interface),
+      explanations_(g_explaining ? new Explanations() : nullptr),
       scan_(state, build_log, deps_log, disk_interface,
-            &config_.depfile_parser_options) {
+            &config_.depfile_parser_options, explanations_.get()) {
   lock_file_path_ = ".ninja_lock";
   string build_dir = state_->bindings_.LookupVariable("builddir");
   if (!build_dir.empty())
     lock_file_path_ = build_dir + "/" + lock_file_path_;
+  status_->SetExplanations(explanations_.get());
 }
 
 Builder::~Builder() {
   Cleanup();
+  status_->SetExplanations(nullptr);
 }
 
 void Builder::Cleanup() {

src/build.h

@@ -22,14 +22,15 @@
 #include <vector>
 
 #include "depfile_parser.h"
-#include "graph.h"
 #include "exit_status.h"
+#include "graph.h"
 #include "util.h"  // int64_t
 
 struct BuildLog;
 struct Builder;
 struct DiskInterface;
 struct Edge;
+struct Explanations;
 struct Node;
 struct State;
 struct Status;
@@ -186,9 +187,8 @@ struct BuildConfig {
 
 /// Builder wraps the build process: starting commands, updating status.
 struct Builder {
-  Builder(State* state, const BuildConfig& config,
-          BuildLog* build_log, DepsLog* deps_log,
-          DiskInterface* disk_interface, Status* status,
+  Builder(State* state, const BuildConfig& config, BuildLog* build_log,
+          DepsLog* deps_log, DiskInterface* disk_interface, Status* status,
           int64_t start_time_millis);
   ~Builder();
 
@@ -242,6 +242,10 @@ struct Builder {
 
   std::string lock_file_path_;
   DiskInterface* disk_interface_;
+
+  // Only create an Explanations class if '-d explain' is used.
+  std::unique_ptr<Explanations> explanations_;
+
   DependencyScan scan_;
 
   // Unimplemented copy ctor and operator= ensure we don't copy the auto_ptr.

src/debug_flags.cc

@@ -26,21 +26,3 @@ bool g_keep_depfile = false;
 bool g_keep_rsp = false;
 
 bool g_experimental_statcache = true;
-
-// Reasons each Node needs rebuilding, for "-d explain".
-typedef std::map<const Node*, std::vector<std::string> > Explanations;
-static Explanations explanations_;
-
-void record_explanation(const Node* node, std::string explanation) {
-  explanations_[node].push_back(explanation);
-}
-
-void print_explanations(FILE *stream, const Edge* edge) {
-  for (std::vector<Node*>::const_iterator o = edge->outputs_.begin();
-       o != edge->outputs_.end(); ++o) {
-    for (std::vector<std::string>::iterator s = explanations_[*o].begin();
-         s != explanations_[*o].end(); ++s) {
-      fprintf(stream, "ninja explain: %s\n", (*s).c_str());
-    }
-  }
-}

src/debug_flags.h

@@ -17,20 +17,10 @@
 
 #include <stdio.h>
 
-#define EXPLAIN(node, fmt, ...) {                                       \
-  if (g_explaining) {                                                   \
-    char buf[1024];                                                     \
-    snprintf(buf, 1024, fmt, __VA_ARGS__);                              \
-    record_explanation(node, buf);                                      \
-  }                                                                     \
-}
-
 struct Edge;
 struct Node;
 
 extern bool g_explaining;
-void record_explanation(const Node* node, std::string reason);
-void print_explanations(FILE *stream, const Edge* node);
 
 extern bool g_keep_depfile;
 

src/disk_interface_test.cc

@@ -259,7 +259,7 @@ TEST_F(DiskInterfaceTest, RemoveDirectory) {
 
 struct StatTest : public StateTestWithBuiltinRules,
                   public DiskInterface {
-  StatTest() : scan_(&state_, NULL, NULL, this, NULL) {}
+  StatTest() : scan_(&state_, NULL, NULL, this, NULL, NULL) {}
 
   // DiskInterface implementation.
   virtual TimeStamp Stat(const string& path, string* err) const;

src/dyndep.cc

@@ -20,6 +20,7 @@
 #include "debug_flags.h"
 #include "disk_interface.h"
 #include "dyndep_parser.h"
+#include "explanations.h"
 #include "graph.h"
 #include "state.h"
 #include "util.h"
@@ -37,7 +38,8 @@ bool DyndepLoader::LoadDyndeps(Node* node, DyndepFile* ddf,
   node->set_dyndep_pending(false);
 
   // Load the dyndep information from the file.
-  EXPLAIN(node, "loading dyndep file '%s'", node->path().c_str());
+  explanations_.Record(node, "loading dyndep file '%s'", node->path().c_str());
+
   if (!LoadDyndepFile(node, ddf, err))
     return false;
 

src/dyndep.h

@@ -19,6 +19,8 @@
 #include <string>
 #include <vector>
 
+#include "explanations.h"
+
 struct DiskInterface;
 struct Edge;
 struct Node;
@@ -42,8 +44,10 @@ struct DyndepFile: public std::map<Edge*, Dyndeps> {};
 /// DyndepLoader loads dynamically discovered dependencies, as
 /// referenced via the "dyndep" attribute in build files.
 struct DyndepLoader {
-  DyndepLoader(State* state, DiskInterface* disk_interface)
-      : state_(state), disk_interface_(disk_interface) {}
+  DyndepLoader(State* state, DiskInterface* disk_interface,
+               Explanations* explanations = nullptr)
+      : state_(state), disk_interface_(disk_interface),
+        explanations_(explanations) {}
 
   /// Load a dyndep file from the given node's path and update the
   /// build graph with the new information.  One overload accepts
@@ -59,6 +63,7 @@ struct DyndepLoader {
 
   State* state_;
   DiskInterface* disk_interface_;
+  mutable OptionalExplanations explanations_;
 };
 
 #endif  // NINJA_DYNDEP_LOADER_H_

src/explanations.h

@@ -0,0 +1,89 @@
+// Copyright 2024 Google Inc. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#pragma once
+
+#include <stdarg.h>
+#include <stdio.h>
+
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+/// A class used to record a list of explanation strings associated
+/// with a given 'item' pointer. This is used to implement the
+/// `-d explain` feature.
+struct Explanations {
+ public:
+  /// Record an explanation for |item| if this instance is enabled.
+  void Record(const void* item, const char* fmt, ...) {
+    va_list args;
+    va_start(args, fmt);
+    RecordArgs(item, fmt, args);
+    va_end(args);
+  }
+
+  /// Same as Record(), but uses a va_list to pass formatting arguments.
+  void RecordArgs(const void* item, const char* fmt, va_list args) {
+    char buffer[1024];
+    vsnprintf(buffer, sizeof(buffer), fmt, args);
+    map_[item].emplace_back(buffer);
+  }
+
+  /// Lookup the explanations recorded for |item|, and append them
+  /// to |*out|, if any.
+  void LookupAndAppend(const void* item, std::vector<std::string>* out) {
+    auto it = map_.find(item);
+    if (it == map_.end())
+      return;
+
+    for (const auto& explanation : it->second)
+      out->push_back(explanation);
+  }
+
+ private:
+  bool enabled_ = false;
+  std::unordered_map<const void*, std::vector<std::string>> map_;
+};
+
+/// Convenience wrapper for an Explanations pointer, which can be null
+/// if no explanations need to be recorded.
+struct OptionalExplanations {
+  OptionalExplanations(Explanations* explanations)
+      : explanations_(explanations) {}
+
+  void Record(const void* item, const char* fmt, ...) {
+    if (explanations_) {
+      va_list args;
+      va_start(args, fmt);
+      explanations_->RecordArgs(item, fmt, args);
+      va_end(args);
+    }
+  }
+
+  void RecordArgs(const void* item, const char* fmt, va_list args) {
+    if (explanations_)
+      explanations_->RecordArgs(item, fmt, args);
+  }
+
+  void LookupAndAppend(const void* item, std::vector<std::string>* out) {
+    if (explanations_)
+      explanations_->LookupAndAppend(item, out);
+  }
+
+  Explanations* ptr() const { return explanations_; }
+
+ private:
+  Explanations* explanations_;
+};

src/explanations_test.cc

@@ -0,0 +1,97 @@
+// Copyright 2024 Google Inc. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "explanations.h"
+
+#include "test.h"
+
+namespace {
+
+const void* MakeItem(size_t v) {
+  return reinterpret_cast<const void*>(v);
+}
+
+}  // namespace
+
+TEST(Explanations, Explanations) {
+  Explanations exp;
+
+  exp.Record(MakeItem(1), "first explanation");
+  exp.Record(MakeItem(1), "second explanation");
+  exp.Record(MakeItem(2), "third explanation");
+  exp.Record(MakeItem(2), "fourth %s", "explanation");
+
+  std::vector<std::string> list;
+
+  exp.LookupAndAppend(MakeItem(0), &list);
+  ASSERT_TRUE(list.empty());
+
+  exp.LookupAndAppend(MakeItem(1), &list);
+  ASSERT_EQ(2u, list.size());
+  EXPECT_EQ(list[0], "first explanation");
+  EXPECT_EQ(list[1], "second explanation");
+
+  exp.LookupAndAppend(MakeItem(2), &list);
+  ASSERT_EQ(4u, list.size());
+  EXPECT_EQ(list[0], "first explanation");
+  EXPECT_EQ(list[1], "second explanation");
+  EXPECT_EQ(list[2], "third explanation");
+  EXPECT_EQ(list[3], "fourth explanation");
+}
+
+TEST(Explanations, OptionalExplanationsNonNull) {
+  Explanations parent;
+  OptionalExplanations exp(&parent);
+
+  exp.Record(MakeItem(1), "first explanation");
+  exp.Record(MakeItem(1), "second explanation");
+  exp.Record(MakeItem(2), "third explanation");
+  exp.Record(MakeItem(2), "fourth %s", "explanation");
+
+  std::vector<std::string> list;
+
+  exp.LookupAndAppend(MakeItem(0), &list);
+  ASSERT_TRUE(list.empty());
+
+  exp.LookupAndAppend(MakeItem(1), &list);
+  ASSERT_EQ(2u, list.size());
+  EXPECT_EQ(list[0], "first explanation");
+  EXPECT_EQ(list[1], "second explanation");
+
+  exp.LookupAndAppend(MakeItem(2), &list);
+  ASSERT_EQ(4u, list.size());
+  EXPECT_EQ(list[0], "first explanation");
+  EXPECT_EQ(list[1], "second explanation");
+  EXPECT_EQ(list[2], "third explanation");
+  EXPECT_EQ(list[3], "fourth explanation");
+}
+
+TEST(Explanations, OptionalExplanationsWithNullPointer) {
+  OptionalExplanations exp(nullptr);
+
+  exp.Record(MakeItem(1), "first explanation");
+  exp.Record(MakeItem(1), "second explanation");
+  exp.Record(MakeItem(2), "third explanation");
+  exp.Record(MakeItem(2), "fourth %s", "explanation");
+
+  std::vector<std::string> list;
+  exp.LookupAndAppend(MakeItem(0), &list);
+  ASSERT_TRUE(list.empty());
+
+  exp.LookupAndAppend(MakeItem(1), &list);
+  ASSERT_TRUE(list.empty());
+
+  exp.LookupAndAppend(MakeItem(2), &list);
+  ASSERT_TRUE(list.empty());
+}