Merge pull request #1060 from bradking/deduplicate-disk-interface

Deduplicate disk abstraction infrastructure

12 files changed, 99 insertions, 91 deletions

diff --git a/src/build.cc b/src/build.cc
index ab2460a..45d3f32 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -871,9 +871,17 @@ bool Builder::ExtractDeps(CommandRunner::Result* result,
       return false;
     }
 
-    string content = disk_interface_->ReadFile(depfile, err);
-    if (!err->empty())
+    // Read depfile content.  Treat a missing depfile as empty.
+    string content;
+    switch (disk_interface_->ReadFile(depfile, &content, err)) {
+    case DiskInterface::Okay:
+      break;
+    case DiskInterface::NotFound:
+      err->clear();
+      break;
+    case DiskInterface::OtherError:
       return false;
+    }
     if (content.empty())
       return true;
 
diff --git a/src/disk_interface.cc b/src/disk_interface.cc
index 70282c0..451a9b4 100644
--- a/src/disk_interface.cc
+++ b/src/disk_interface.cc
@@ -229,14 +229,14 @@ bool RealDiskInterface::MakeDir(const string& path) {
   return true;
 }
 
-string RealDiskInterface::ReadFile(const string& path, string* err) {
-  string contents;
-  int ret = ::ReadFile(path, &contents, err);
-  if (ret == -ENOENT) {
-    // Swallow ENOENT.
-    err->clear();
+FileReader::Status RealDiskInterface::ReadFile(const string& path,
+                                               string* contents,
+                                               string* err) {
+  switch (::ReadFile(path, contents, err)) {
+  case 0:       return Okay;
+  case -ENOENT: return NotFound;
+  default:      return OtherError;
   }
-  return contents;
 }
 
 int RealDiskInterface::RemoveFile(const string& path) {
diff --git a/src/disk_interface.h b/src/disk_interface.h
index b61d192..145e089 100644
--- a/src/disk_interface.h
+++ b/src/disk_interface.h
@@ -21,13 +21,29 @@ using namespace std;
 
 #include "timestamp.h"
 
+/// Interface for reading files from disk.  See DiskInterface for details.
+/// This base offers the minimum interface needed just to read files.
+struct FileReader {
+  virtual ~FileReader() {}
+
+  /// Result of ReadFile.
+  enum Status {
+    Okay,
+    NotFound,
+    OtherError
+  };
+
+  /// Read and store in given string.  On success, return Okay.
+  /// On error, return another Status and fill |err|.
+  virtual Status ReadFile(const string& path, string* contents,
+                          string* err) = 0;
+};
+
 /// Interface for accessing the disk.
 ///
 /// Abstract so it can be mocked out for tests.  The real implementation
 /// is RealDiskInterface.
-struct DiskInterface {
-  virtual ~DiskInterface() {}
-
+struct DiskInterface: public FileReader {
   /// stat() a file, returning the mtime, or 0 if missing and -1 on
   /// other errors.
   virtual TimeStamp Stat(const string& path, string* err) const = 0;
@@ -39,9 +55,6 @@ struct DiskInterface {
   /// Returns true on success, false on failure
   virtual bool WriteFile(const string& path, const string& contents) = 0;
 
-  /// Read a file to a string.  Fill in |err| on error.
-  virtual string ReadFile(const string& path, string* err) = 0;
-
   /// Remove the file named @a path. It behaves like 'rm -f path' so no errors
   /// are reported if it does not exists.
   /// @returns 0 if the file has been removed,
@@ -65,7 +78,7 @@ struct RealDiskInterface : public DiskInterface {
   virtual TimeStamp Stat(const string& path, string* err) const;
   virtual bool MakeDir(const string& path);
   virtual bool WriteFile(const string& path, const string& contents);
-  virtual string ReadFile(const string& path, string* err);
+  virtual Status ReadFile(const string& path, string* contents, string* err);
   virtual int RemoveFile(const string& path);
 
   /// Whether stat information can be cached.  Only has an effect on Windows.
diff --git a/src/disk_interface_test.cc b/src/disk_interface_test.cc
index 9d210b4..7187bdf 100644
--- a/src/disk_interface_test.cc
+++ b/src/disk_interface_test.cc
@@ -157,8 +157,12 @@ TEST_F(DiskInterfaceTest, StatCache) {
 
 TEST_F(DiskInterfaceTest, ReadFile) {
   string err;
-  EXPECT_EQ("", disk_.ReadFile("foobar", &err));
-  EXPECT_EQ("", err);
+  std::string content;
+  ASSERT_EQ(DiskInterface::NotFound,
+            disk_.ReadFile("foobar", &content, &err));
+  EXPECT_EQ("", content);
+  EXPECT_NE("", err); // actual value is platform-specific
+  err.clear();
 
   const char* kTestFile = "testfile";
   FILE* f = fopen(kTestFile, "wb");
@@ -167,7 +171,9 @@ TEST_F(DiskInterfaceTest, ReadFile) {
   fprintf(f, "%s", kTestContent);
   ASSERT_EQ(0, fclose(f));
 
-  EXPECT_EQ(kTestContent, disk_.ReadFile(kTestFile, &err));
+  ASSERT_EQ(DiskInterface::Okay,
+            disk_.ReadFile(kTestFile, &content, &err));
+  EXPECT_EQ(kTestContent, content);
   EXPECT_EQ("", err);
 }
 
@@ -208,9 +214,9 @@ struct StatTest : public StateTestWithBuiltinRules,
     assert(false);
     return false;
   }
-  virtual string ReadFile(const string& path, string* err) {
+  virtual Status ReadFile(const string& path, string* contents, string* err) {
     assert(false);
-    return "";
+    return NotFound;
   }
   virtual int RemoveFile(const string& path) {
     assert(false);
diff --git a/src/graph.cc b/src/graph.cc
index 9e65675..98f1461 100644
--- a/src/graph.cc
+++ b/src/graph.cc
@@ -395,8 +395,15 @@ bool ImplicitDepLoader::LoadDeps(Edge* edge, string* err) {
 bool ImplicitDepLoader::LoadDepFile(Edge* edge, const string& path,
                                     string* err) {
   METRIC_RECORD("depfile load");
-  string content = disk_interface_->ReadFile(path, err);
-  if (!err->empty()) {
+  // Read depfile content.  Treat a missing depfile as empty.
+  string content;
+  switch (disk_interface_->ReadFile(path, &content, err)) {
+  case DiskInterface::Okay:
+    break;
+  case DiskInterface::NotFound:
+    err->clear();
+    break;
+  case DiskInterface::OtherError:
     *err = "loading '" + path + "': " + *err;
     return false;
   }
diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc
index d0fac59..8bdb330 100644
--- a/src/manifest_parser.cc
+++ b/src/manifest_parser.cc
@@ -18,6 +18,7 @@
 #include <stdlib.h>
 #include <vector>
 
+#include "disk_interface.h"
 #include "graph.h"
 #include "metrics.h"
 #include "state.h"
@@ -35,7 +36,7 @@ bool ManifestParser::Load(const string& filename, string* err, Lexer* parent) {
   METRIC_RECORD(".ninja parse");
   string contents;
   string read_err;
-  if (!file_reader_->ReadFile(filename, &contents, &read_err)) {
+  if (file_reader_->ReadFile(filename, &contents, &read_err) != FileReader::Okay) {
     *err = "loading '" + filename + "': " + read_err;
     if (parent)
       parent->Error(string(*err), err);
diff --git a/src/manifest_parser.h b/src/manifest_parser.h
index 41d388c..043e4b2 100644
--- a/src/manifest_parser.h
+++ b/src/manifest_parser.h
@@ -23,6 +23,7 @@ using namespace std;
 
 struct BindingEnv;
 struct EvalString;
+struct FileReader;
 struct State;
 
 enum DupeEdgeAction {
@@ -32,11 +33,6 @@ enum DupeEdgeAction {
 
 /// Parses .ninja files.
 struct ManifestParser {
-  struct FileReader {
-    virtual ~FileReader() {}
-    virtual bool ReadFile(const string& path, string* content, string* err) = 0;
-  };
-
   ManifestParser(State* state, FileReader* file_reader,
                  DupeEdgeAction dupe_edge_action);
 
diff --git a/src/manifest_parser_perftest.cc b/src/manifest_parser_perftest.cc
index ef3b663..572e2d5 100644
--- a/src/manifest_parser_perftest.cc
+++ b/src/manifest_parser_perftest.cc
@@ -36,12 +36,6 @@
 #include "state.h"
 #include "util.h"
 
-struct RealFileReader : public ManifestParser::FileReader {
-  virtual bool ReadFile(const string& path, string* content, string* err) {
-    return ::ReadFile(path, content, err) == 0;
-  }
-};
-
 bool WriteFakeManifests(const string& dir, string* err) {
   RealDiskInterface disk_interface;
   TimeStamp mtime = disk_interface.Stat(dir + "/build.ninja", err);
@@ -59,9 +53,9 @@ bool WriteFakeManifests(const string& dir, string* err) {
 
 int LoadManifests(bool measure_command_evaluation) {
   string err;
-  RealFileReader file_reader;
+  RealDiskInterface disk_interface;
   State state;
-  ManifestParser parser(&state, &file_reader, kDupeEdgeActionWarn);
+  ManifestParser parser(&state, &disk_interface, kDupeEdgeActionWarn);
   if (!parser.Load("build.ninja", &err)) {
     fprintf(stderr, "Failed to read test data: %s\n", err.c_str());
     exit(1);
diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc
index a18433a..cb9f405 100644
--- a/src/manifest_parser_test.cc
+++ b/src/manifest_parser_test.cc
@@ -21,30 +21,17 @@
 #include "state.h"
 #include "test.h"
 
-struct ParserTest : public testing::Test,
-                    public ManifestParser::FileReader {
+struct ParserTest : public testing::Test {
   void AssertParse(const char* input) {
-    ManifestParser parser(&state, this, kDupeEdgeActionWarn);
+    ManifestParser parser(&state, &fs_, kDupeEdgeActionWarn);
     string err;
     EXPECT_TRUE(parser.ParseTest(input, &err));
     ASSERT_EQ("", err);
     VerifyGraph(state);
   }
 
-  virtual bool ReadFile(const string& path, string* content, string* err) {
-    files_read_.push_back(path);
-    map<string, string>::iterator i = files_.find(path);
-    if (i == files_.end()) {
-      *err = "No such file or directory";  // Match strerror() for ENOENT.
-      return false;
-    }
-    *content = i->second;
-    return true;
-  }
-
   State state;
-  map<string, string> files_;
-  vector<string> files_read_;
+  VirtualFileSystem fs_;
 };
 
 TEST_F(ParserTest, Empty) {
@@ -371,22 +358,22 @@ TEST_F(ParserTest, DuplicateEdgeWithMultipleOutputsError) {
 "build out1 out2: cat in1\n"
 "build out1: cat in2\n"
 "build final: cat out1\n";
-  ManifestParser parser(&state, this, kDupeEdgeActionError);
+  ManifestParser parser(&state, &fs_, kDupeEdgeActionError);
   string err;
   EXPECT_FALSE(parser.ParseTest(kInput, &err));
   EXPECT_EQ("input:5: multiple rules generate out1 [-w dupbuild=err]\n", err);
 }
 
 TEST_F(ParserTest, DuplicateEdgeInIncludedFile) {
-  files_["sub.ninja"] =
+  fs_.Create("sub.ninja",
     "rule cat\n"
     "  command = cat $in > $out\n"
     "build out1 out2: cat in1\n"
     "build out1: cat in2\n"
-    "build final: cat out1\n";
+    "build final: cat out1\n");
   const char kInput[] =
     "subninja sub.ninja\n";
-  ManifestParser parser(&state, this, kDupeEdgeActionError);
+  ManifestParser parser(&state, &fs_, kDupeEdgeActionError);
   string err;
   EXPECT_FALSE(parser.ParseTest(kInput, &err));
   EXPECT_EQ("sub.ninja:5: multiple rules generate out1 [-w dupbuild=err]\n",
@@ -813,7 +800,7 @@ TEST_F(ParserTest, Errors) {
 
 TEST_F(ParserTest, MissingInput) {
   State state;
-  ManifestParser parser(&state, this, kDupeEdgeActionWarn);
+  ManifestParser parser(&state, &fs_, kDupeEdgeActionWarn);
   string err;
   EXPECT_FALSE(parser.Load("build.ninja", &err));
   EXPECT_EQ("loading 'build.ninja': No such file or directory", err);
@@ -841,9 +828,9 @@ TEST_F(ParserTest, MultipleOutputsWithDeps) {
 }
 
 TEST_F(ParserTest, SubNinja) {
-  files_["test.ninja"] =
+  fs_.Create("test.ninja",
     "var = inner\n"
-    "build $builddir/inner: varref\n";
+    "build $builddir/inner: varref\n");
   ASSERT_NO_FATAL_FAILURE(AssertParse(
 "builddir = some_dir/\n"
 "rule varref\n"
@@ -852,9 +839,9 @@ TEST_F(ParserTest, SubNinja) {
 "build $builddir/outer: varref\n"
 "subninja test.ninja\n"
 "build $builddir/outer2: varref\n"));
-  ASSERT_EQ(1u, files_read_.size());
+  ASSERT_EQ(1u, fs_.files_read_.size());
 
-  EXPECT_EQ("test.ninja", files_read_[0]);
+  EXPECT_EQ("test.ninja", fs_.files_read_[0]);
   EXPECT_TRUE(state.LookupNode("some_dir/outer"));
   // Verify our builddir setting is inherited.
   EXPECT_TRUE(state.LookupNode("some_dir/inner"));
@@ -866,7 +853,7 @@ TEST_F(ParserTest, SubNinja) {
 }
 
 TEST_F(ParserTest, MissingSubNinja) {
-  ManifestParser parser(&state, this, kDupeEdgeActionWarn);
+  ManifestParser parser(&state, &fs_, kDupeEdgeActionWarn);
   string err;
   EXPECT_FALSE(parser.ParseTest("subninja foo.ninja\n", &err));
   EXPECT_EQ("input:1: loading 'foo.ninja': No such file or directory\n"
@@ -877,9 +864,9 @@ TEST_F(ParserTest, MissingSubNinja) {
 
 TEST_F(ParserTest, DuplicateRuleInDifferentSubninjas) {
   // Test that rules are scoped to subninjas.
-  files_["test.ninja"] = "rule cat\n"
-                         "  command = cat\n";
-  ManifestParser parser(&state, this, kDupeEdgeActionWarn);
+  fs_.Create("test.ninja", "rule cat\n"
+                         "  command = cat\n");
+  ManifestParser parser(&state, &fs_, kDupeEdgeActionWarn);
   string err;
   EXPECT_TRUE(parser.ParseTest("rule cat\n"
                                 "  command = cat\n"
@@ -888,11 +875,11 @@ TEST_F(ParserTest, DuplicateRuleInDifferentSubninjas) {
 
 TEST_F(ParserTest, DuplicateRuleInDifferentSubninjasWithInclude) {
   // Test that rules are scoped to subninjas even with includes.
-  files_["rules.ninja"] = "rule cat\n"
-                         "  command = cat\n";
-  files_["test.ninja"] = "include rules.ninja\n"
-                         "build x : cat\n";
-  ManifestParser parser(&state, this, kDupeEdgeActionWarn);
+  fs_.Create("rules.ninja", "rule cat\n"
+                         "  command = cat\n");
+  fs_.Create("test.ninja", "include rules.ninja\n"
+                         "build x : cat\n");
+  ManifestParser parser(&state, &fs_, kDupeEdgeActionWarn);
   string err;
   EXPECT_TRUE(parser.ParseTest("include rules.ninja\n"
                                 "subninja test.ninja\n"
@@ -900,19 +887,19 @@ TEST_F(ParserTest, DuplicateRuleInDifferentSubninjasWithInclude) {
 }
 
 TEST_F(ParserTest, Include) {
-  files_["include.ninja"] = "var = inner\n";
+  fs_.Create("include.ninja", "var = inner\n");
   ASSERT_NO_FATAL_FAILURE(AssertParse(
 "var = outer\n"
 "include include.ninja\n"));
 
-  ASSERT_EQ(1u, files_read_.size());
-  EXPECT_EQ("include.ninja", files_read_[0]);
+  ASSERT_EQ(1u, fs_.files_read_.size());
+  EXPECT_EQ("include.ninja", fs_.files_read_[0]);
   EXPECT_EQ("inner", state.bindings_.LookupVariable("var"));
 }
 
 TEST_F(ParserTest, BrokenInclude) {
-  files_["include.ninja"] = "build\n";
-  ManifestParser parser(&state, this, kDupeEdgeActionWarn);
+  fs_.Create("include.ninja", "build\n");
+  ManifestParser parser(&state, &fs_, kDupeEdgeActionWarn);
   string err;
   EXPECT_FALSE(parser.ParseTest("include include.ninja\n", &err));
   EXPECT_EQ("include.ninja:1: expected path\n"
diff --git a/src/ninja.cc b/src/ninja.cc
index 3af10d6..a3f1be0 100644
--- a/src/ninja.cc
+++ b/src/ninja.cc
@@ -229,14 +229,6 @@ int GuessParallelism() {
   }
 }
 
-/// An implementation of ManifestParser::FileReader that actually reads
-/// the file.
-struct RealFileReader : public ManifestParser::FileReader {
-  virtual bool ReadFile(const string& path, string* content, string* err) {
-    return ::ReadFile(path, content, err) == 0;
-  }
-};
-
 /// Rebuild the build manifest, if necessary.
 /// Returns true if the manifest was rebuilt.
 bool NinjaMain::RebuildManifest(const char* input_file, string* err) {
@@ -1117,8 +1109,7 @@ int real_main(int argc, char** argv) {
   for (int cycle = 1; cycle <= kCycleLimit; ++cycle) {
     NinjaMain ninja(ninja_command, config);
 
-    RealFileReader file_reader;
-    ManifestParser parser(&ninja.state_, &file_reader,
+    ManifestParser parser(&ninja.state_, &ninja.disk_interface_,
                           options.dupe_edges_should_err
                               ? kDupeEdgeActionError
                               : kDupeEdgeActionWarn);
diff --git a/src/test.cc b/src/test.cc
index 841ce04..53bfc48 100644
--- a/src/test.cc
+++ b/src/test.cc
@@ -164,12 +164,17 @@ bool VirtualFileSystem::MakeDir(const string& path) {
   return true;  // success
 }
 
-string VirtualFileSystem::ReadFile(const string& path, string* err) {
+FileReader::Status VirtualFileSystem::ReadFile(const string& path,
+                                               string* contents,
+                                               string* err) {
   files_read_.push_back(path);
   FileMap::iterator i = files_.find(path);
-  if (i != files_.end())
-    return i->second.contents;
-  return "";
+  if (i != files_.end()) {
+    *contents = i->second.contents;
+    return Okay;
+  }
+  *err = strerror(ENOENT);
+  return NotFound;
 }
 
 int VirtualFileSystem::RemoveFile(const string& path) {
diff --git a/src/test.h b/src/test.h
index 156e68a..488c243 100644
--- a/src/test.h
+++ b/src/test.h
@@ -145,7 +145,7 @@ struct VirtualFileSystem : public DiskInterface {
   virtual TimeStamp Stat(const string& path, string* err) const;
   virtual bool WriteFile(const string& path, const string& contents);
   virtual bool MakeDir(const string& path);
-  virtual string ReadFile(const string& path, string* err);
+  virtual Status ReadFile(const string& path, string* contents, string* err);
   virtual int RemoveFile(const string& path);
 
   /// An entry for a single in-memory file.