commit 987196fc15fbb5feacdcd7de1a19bfdbdce86438
author Takuto Ikuta <tikuta@google.com> Fri Mar 25 19:08:25 2022 +0900
committer Commit Bot <commit-bot@chromium.org> Fri Mar 25 19:44:51 2022 +0000
tree d62fb5000f71f02262c983b8786b3cad7f448c34
parent dcd874cd2e10d576c39160a63c1176a77ea07e3b

use unique_ptr for TraceItem

Also this guards event_ access by mutex.

Change-Id: I71a67546c8b3474775e3eac4b33a6e0f188791c7
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/13320
Reviewed-by: Brett Wilson <brettw@chromium.org>
Commit-Queue: Brett Wilson <brettw@chromium.org>

src/gn/import_manager.cc

diff --git a/src/gn/import_manager.cc b/src/gn/import_manager.cc
index a4928bf..69f44b4 100644
--- a/src/gn/import_manager.cc
+++ b/src/gn/import_manager.cc
@@ -125,14 +125,14 @@
       if (TracingEnabled() &&
           TicksDelta(import_block_end, import_block_begin).InMilliseconds() >
               kImportBlockTraceThresholdMS) {
-        auto* import_block_trace =
-            new TraceItem(TraceItem::TRACE_IMPORT_BLOCK, file.value(),
-                          std::this_thread::get_id());
+        auto import_block_trace = std::make_unique<TraceItem>(
+            TraceItem::TRACE_IMPORT_BLOCK, file.value(),
+            std::this_thread::get_id());
         import_block_trace->set_begin(import_block_begin);
         import_block_trace->set_end(import_block_end);
         import_block_trace->set_toolchain(
             scope->settings()->toolchain_label().GetUserVisibleName(false));
-        AddTrace(import_block_trace);
+        AddTrace(std::move(import_block_trace));
       }
     }
 

src/gn/trace.cc

diff --git a/src/gn/trace.cc b/src/gn/trace.cc
index c6eeed2..f812710 100644
--- a/src/gn/trace.cc
+++ b/src/gn/trace.cc
@@ -30,18 +30,25 @@
   TraceLog() { events_.reserve(16384); }
   // Trace items leaked intentionally.
 
-  void Add(TraceItem* item) {
+  void Add(std::unique_ptr<TraceItem> item) {
     std::lock_guard<std::mutex> lock(lock_);
-    events_.push_back(item);
+    events_.push_back(std::move(item));
   }
 
   // Returns a copy for threadsafety.
-  std::vector<TraceItem*> events() const { return events_; }
+  std::vector<TraceItem*> events() const {
+    std::vector<TraceItem*> events;
+    std::lock_guard<std::mutex> lock(lock_);
+    events.reserve(events_.size());
+    for (const auto& e : events_)
+      events.push_back(e.get());
+    return events;
+  }
 
  private:
-  std::mutex lock_;
+  mutable std::mutex lock_;
 
-  std::vector<TraceItem*> events_;
+  std::vector<std::unique_ptr<TraceItem>> events_;
 
   TraceLog(const TraceLog&) = delete;
   TraceLog& operator=(const TraceLog&) = delete;
@@ -120,18 +127,17 @@
 TraceItem::~TraceItem() = default;
 
 ScopedTrace::ScopedTrace(TraceItem::Type t, const std::string& name)
-    : item_(nullptr), done_(false) {
+    : done_(false) {
   if (trace_log) {
-    item_ = new TraceItem(t, name, std::this_thread::get_id());
+    item_ = std::make_unique<TraceItem>(t, name, std::this_thread::get_id());
     item_->set_begin(TicksNow());
   }
 }
 
-ScopedTrace::ScopedTrace(TraceItem::Type t, const Label& label)
-    : item_(nullptr), done_(false) {
+ScopedTrace::ScopedTrace(TraceItem::Type t, const Label& label) : done_(false) {
   if (trace_log) {
-    item_ = new TraceItem(t, label.GetUserVisibleName(false),
-                          std::this_thread::get_id());
+    item_ = std::make_unique<TraceItem>(t, label.GetUserVisibleName(false),
+                                        std::this_thread::get_id());
     item_->set_begin(TicksNow());
   }
 }
@@ -155,7 +161,7 @@
     done_ = true;
     if (trace_log) {
       item_->set_end(TicksNow());
-      AddTrace(item_);
+      AddTrace(std::move(item_));
     }
   }
 }
@@ -169,8 +175,8 @@
   return !!trace_log;
 }
 
-void AddTrace(TraceItem* item) {
-  trace_log->Add(item);
+void AddTrace(std::unique_ptr<TraceItem> item) {
+  trace_log->Add(std::move(item));
 }
 
 std::string SummarizeTraces() {

src/gn/trace.h

diff --git a/src/gn/trace.h b/src/gn/trace.h
index b89e82a..b59cd34 100644
--- a/src/gn/trace.h
+++ b/src/gn/trace.h
@@ -5,6 +5,7 @@
 #ifndef TOOLS_GN_TRACE_H_
 #define TOOLS_GN_TRACE_H_
 
+#include <memory>
 #include <string>
 #include <thread>
 
@@ -80,7 +81,7 @@
   void Done();
 
  private:
-  TraceItem* item_;
+  std::unique_ptr<TraceItem> item_;
   bool done_;
 };
 
@@ -90,8 +91,8 @@
 // Returns whether tracing is enabled.
 bool TracingEnabled();
 
-// Adds a trace event to the log. Takes ownership of the pointer.
-void AddTrace(TraceItem* item);
+// Adds a trace event to the log.
+void AddTrace(std::unique_ptr<TraceItem> item);
 
 // Returns a summary of the current traces, or the empty string if tracing is
 // not enabled.