Merge pull request #2534 from digit-google/fix-build-log-leak

Fix build log leak
diff --git a/src/build_log.cc b/src/build_log.cc
index 073d2fe..726949a 100644
--- a/src/build_log.cc
+++ b/src/build_log.cc
@@ -41,8 +41,6 @@
 #define strtoll _strtoi64
 #endif
 
-using namespace std;
-
 // Implementation details:
 // Each run's log appends to the log file.
 // To load, we run through all log entries in series, throwing away
@@ -63,24 +61,21 @@
   return rapidhash(command.str_, command.len_);
 }
 
-BuildLog::LogEntry::LogEntry(const string& output)
-  : output(output) {}
+BuildLog::LogEntry::LogEntry(std::string output) : output(std::move(output)) {}
 
-BuildLog::LogEntry::LogEntry(const string& output, uint64_t command_hash,
-  int start_time, int end_time, TimeStamp mtime)
-  : output(output), command_hash(command_hash),
-    start_time(start_time), end_time(end_time), mtime(mtime)
-{}
+BuildLog::LogEntry::LogEntry(const std::string& output, uint64_t command_hash,
+                             int start_time, int end_time, TimeStamp mtime)
+    : output(output), command_hash(command_hash), start_time(start_time),
+      end_time(end_time), mtime(mtime) {}
 
-BuildLog::BuildLog()
-  : log_file_(NULL), needs_recompaction_(false) {}
+BuildLog::BuildLog() = default;
 
 BuildLog::~BuildLog() {
   Close();
 }
 
-bool BuildLog::OpenForWrite(const string& path, const BuildLogUser& user,
-                            string* err) {
+bool BuildLog::OpenForWrite(const std::string& path, const BuildLogUser& user,
+                            std::string* err) {
   if (needs_recompaction_) {
     if (!Recompact(path, user, err))
       return false;
@@ -94,18 +89,19 @@
 
 bool BuildLog::RecordCommand(Edge* edge, int start_time, int end_time,
                              TimeStamp mtime) {
-  string command = edge->EvaluateCommand(true);
+  std::string command = edge->EvaluateCommand(true);
   uint64_t command_hash = LogEntry::HashCommand(command);
-  for (vector<Node*>::iterator out = edge->outputs_.begin();
+  for (std::vector<Node*>::iterator out = edge->outputs_.begin();
        out != edge->outputs_.end(); ++out) {
-    const string& path = (*out)->path();
+    const std::string& path = (*out)->path();
     Entries::iterator i = entries_.find(path);
     LogEntry* log_entry;
     if (i != entries_.end()) {
-      log_entry = i->second;
+      log_entry = i->second.get();
     } else {
       log_entry = new LogEntry(path);
-      entries_.insert(Entries::value_type(log_entry->output, log_entry));
+      // Passes ownership of |log_entry| to the map, but keeps the pointer valid.
+      entries_.emplace(log_entry->output, std::unique_ptr<LogEntry>(log_entry));
     }
     log_entry->command_hash = command_hash;
     log_entry->start_time = start_time;
@@ -209,7 +205,7 @@
   char* line_end_;
 };
 
-LoadStatus BuildLog::Load(const string& path, string* err) {
+LoadStatus BuildLog::Load(const std::string& path, std::string* err) {
   METRIC_RECORD(".ninja_log load");
   FILE* file = fopen(path.c_str(), "r");
   if (!file) {
@@ -283,7 +279,7 @@
     end = static_cast<char*>(memchr(start, kFieldSeparator, line_end - start));
     if (!end)
       continue;
-    string output = string(start, end - start);
+    std::string output(start, end - start);
 
     start = end + 1;
     end = line_end;
@@ -291,10 +287,11 @@
     LogEntry* entry;
     Entries::iterator i = entries_.find(output);
     if (i != entries_.end()) {
-      entry = i->second;
+      entry = i->second.get();
     } else {
-      entry = new LogEntry(output);
-      entries_.insert(Entries::value_type(entry->output, entry));
+      entry = new LogEntry(std::move(output));
+      // Passes ownership of |entry| to the map, but keeps the pointer valid.
+      entries_.emplace(entry->output, std::unique_ptr<LogEntry>(entry));
       ++unique_entry_count;
     }
     ++total_entry_count;
@@ -327,10 +324,10 @@
   return LOAD_SUCCESS;
 }
 
-BuildLog::LogEntry* BuildLog::LookupByOutput(const string& path) {
+BuildLog::LogEntry* BuildLog::LookupByOutput(const std::string& path) {
   Entries::iterator i = entries_.find(path);
   if (i != entries_.end())
-    return i->second;
+    return i->second.get();
   return NULL;
 }
 
@@ -340,12 +337,12 @@
           entry.output.c_str(), entry.command_hash) > 0;
 }
 
-bool BuildLog::Recompact(const string& path, const BuildLogUser& user,
-                         string* err) {
+bool BuildLog::Recompact(const std::string& path, const BuildLogUser& user,
+                         std::string* err) {
   METRIC_RECORD(".ninja_log recompact");
 
   Close();
-  string temp_path = path + ".recompact";
+  std::string temp_path = path + ".recompact";
   FILE* f = fopen(temp_path.c_str(), "wb");
   if (!f) {
     *err = strerror(errno);
@@ -358,22 +355,22 @@
     return false;
   }
 
-  vector<StringPiece> dead_outputs;
-  for (Entries::iterator i = entries_.begin(); i != entries_.end(); ++i) {
-    if (user.IsPathDead(i->first)) {
-      dead_outputs.push_back(i->first);
+  std::vector<StringPiece> dead_outputs;
+  for (const auto& pair : entries_) {
+    if (user.IsPathDead(pair.first)) {
+      dead_outputs.push_back(pair.first);
       continue;
     }
 
-    if (!WriteEntry(f, *i->second)) {
+    if (!WriteEntry(f, *pair.second)) {
       *err = strerror(errno);
       fclose(f);
       return false;
     }
   }
 
-  for (size_t i = 0; i < dead_outputs.size(); ++i)
-    entries_.erase(dead_outputs[i]);
+  for (StringPiece output : dead_outputs)
+    entries_.erase(output);
 
   fclose(f);
   if (unlink(path.c_str()) < 0) {
@@ -408,24 +405,24 @@
     fclose(f);
     return false;
   }
-  for (Entries::iterator i = entries_.begin(); i != entries_.end(); ++i) {
+  for (auto& pair : entries_) {
     bool skip = output_count > 0;
     for (int j = 0; j < output_count; ++j) {
-      if (i->second->output == outputs[j]) {
+      if (pair.second->output == outputs[j]) {
         skip = false;
         break;
       }
     }
     if (!skip) {
-      const TimeStamp mtime = disk_interface.Stat(i->second->output, err);
+      const TimeStamp mtime = disk_interface.Stat(pair.second->output, err);
       if (mtime == -1) {
         fclose(f);
         return false;
       }
-      i->second->mtime = mtime;
+      pair.second->mtime = mtime;
     }
 
-    if (!WriteEntry(f, *i->second)) {
+    if (!WriteEntry(f, *pair.second)) {
       *err = strerror(errno);
       fclose(f);
       return false;
diff --git a/src/build_log.h b/src/build_log.h
index 1223c2a..61bd8ff 100644
--- a/src/build_log.h
+++ b/src/build_log.h
@@ -15,9 +15,11 @@
 #ifndef NINJA_BUILD_LOG_H_
 #define NINJA_BUILD_LOG_H_
 
-#include <string>
 #include <stdio.h>
 
+#include <memory>
+#include <string>
+
 #include "hash_map.h"
 #include "load_status.h"
 #include "timestamp.h"
@@ -57,10 +59,10 @@
 
   struct LogEntry {
     std::string output;
-    uint64_t command_hash;
-    int start_time;
-    int end_time;
-    TimeStamp mtime;
+    uint64_t command_hash = 0;
+    int start_time = 0;
+    int end_time = 0;
+    TimeStamp mtime = 0;
 
     static uint64_t HashCommand(StringPiece command);
 
@@ -71,7 +73,7 @@
           mtime == o.mtime;
     }
 
-    explicit LogEntry(const std::string& output);
+    explicit LogEntry(std::string output);
     LogEntry(const std::string& output, uint64_t command_hash,
              int start_time, int end_time, TimeStamp mtime);
   };
@@ -90,7 +92,7 @@
   bool Restat(StringPiece path, const DiskInterface& disk_interface,
               int output_count, char** outputs, std::string* err);
 
-  typedef ExternalStringHashMap<LogEntry*>::Type Entries;
+  typedef ExternalStringHashMap<std::unique_ptr<LogEntry>>::Type Entries;
   const Entries& entries() const { return entries_; }
 
  private:
@@ -99,9 +101,9 @@
   bool OpenForWriteIfNeeded();
 
   Entries entries_;
-  FILE* log_file_;
+  FILE* log_file_ = nullptr;
   std::string log_file_path_;
-  bool needs_recompaction_;
+  bool needs_recompaction_ = false;
 };
 
 #endif // NINJA_BUILD_LOG_H_
diff --git a/src/build_log_test.cc b/src/build_log_test.cc
index 630b1f1..2b0d6cd 100644
--- a/src/build_log_test.cc
+++ b/src/build_log_test.cc
@@ -27,8 +27,6 @@
 #endif
 #include <cassert>
 
-using namespace std;
-
 namespace {
 
 const char kTestFilename[] = "BuildLogTest-tempfile";
@@ -50,7 +48,7 @@
 "build mid: cat in\n");
 
   BuildLog log1;
-  string err;
+  std::string err;
   EXPECT_TRUE(log1.OpenForWrite(kTestFilename, *this, &err));
   ASSERT_EQ("", err);
   log1.RecordCommand(state_.edges_[0], 15, 18);
@@ -77,7 +75,7 @@
   const size_t kVersionPos = strlen(kExpectedVersion) - 2;  // Points at 'X'.
 
   BuildLog log;
-  string contents, err;
+  std::string contents, err;
 
   EXPECT_TRUE(log.OpenForWrite(kTestFilename, *this, &err));
   ASSERT_EQ("", err);
@@ -111,7 +109,7 @@
       BuildLog::LogEntry::HashCommand("command def"));
   fclose(f);
 
-  string err;
+  std::string err;
   BuildLog log;
   EXPECT_TRUE(log.Load(kTestFilename, &err));
   ASSERT_EQ("", err);
@@ -128,7 +126,7 @@
 
   {
     BuildLog log1;
-    string err;
+    std::string err;
     EXPECT_TRUE(log1.OpenForWrite(kTestFilename, *this, &err));
     ASSERT_EQ("", err);
     log1.RecordCommand(state_.edges_[0], 15, 18);
@@ -148,7 +146,7 @@
   // crash when parsing.
   for (off_t size = statbuf.st_size; size > 0; --size) {
     BuildLog log2;
-    string err;
+    std::string err;
     EXPECT_TRUE(log2.OpenForWrite(kTestFilename, *this, &err));
     ASSERT_EQ("", err);
     log2.RecordCommand(state_.edges_[0], 15, 18);
@@ -169,10 +167,10 @@
   fprintf(f, "123 456 0 out command\n");
   fclose(f);
 
-  string err;
+  std::string err;
   BuildLog log;
   EXPECT_TRUE(log.Load(kTestFilename, &err));
-  ASSERT_NE(err.find("version"), string::npos);
+  ASSERT_NE(err.find("version"), std::string::npos);
 }
 
 TEST_F(BuildLogTest, SpacesInOutput) {
@@ -182,7 +180,7 @@
       BuildLog::LogEntry::HashCommand("command"));
   fclose(f);
 
-  string err;
+  std::string err;
   BuildLog log;
   EXPECT_TRUE(log.Load(kTestFilename, &err));
   ASSERT_EQ("", err);
@@ -208,7 +206,7 @@
       BuildLog::LogEntry::HashCommand("command2"));
   fclose(f);
 
-  string err;
+  std::string err;
   BuildLog log;
   EXPECT_TRUE(log.Load(kTestFilename, &err));
   ASSERT_EQ("", err);
@@ -229,22 +227,23 @@
 }
 
 struct TestDiskInterface : public DiskInterface {
-  virtual TimeStamp Stat(const string& path, string* err) const {
+  virtual TimeStamp Stat(const std::string& path, std::string* err) const {
     return 4;
   }
-  virtual bool WriteFile(const string& path, const string& contents) {
+  virtual bool WriteFile(const std::string& path, const std::string& contents) {
     assert(false);
     return true;
   }
-  virtual bool MakeDir(const string& path) {
+  virtual bool MakeDir(const std::string& path) {
     assert(false);
     return false;
   }
-  virtual Status ReadFile(const string& path, string* contents, string* err) {
+  virtual Status ReadFile(const std::string& path, std::string* contents,
+                          std::string* err) {
     assert(false);
     return NotFound;
   }
-  virtual int RemoveFile(const string& path) {
+  virtual int RemoveFile(const std::string& path) {
     assert(false);
     return 0;
   }
@@ -289,7 +288,7 @@
       BuildLog::LogEntry::HashCommand("command2"));
   fclose(f);
 
-  string err;
+  std::string err;
   BuildLog log;
   EXPECT_TRUE(log.Load(kTestFilename, &err));
   ASSERT_EQ("", err);
@@ -335,7 +334,7 @@
 "build out2: cat in\n");
 
   BuildLog log1;
-  string err;
+  std::string err;
   EXPECT_TRUE(log1.OpenForWrite(kTestFilename, *this, &err));
   ASSERT_EQ("", err);
   // Record the same edge several times, to trigger recompaction