Replace ManifestParser::FileReader with general FileReader

Avoid having two separate filesystem interfaces.  Simplify test
infrastructure by avoiding custom `ManifestParser::FileReader`
implementations.

Author: bradking

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);

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);
 

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);

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,31 +875,31 @@ 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"
                                 "build y : cat\n", &err));
 }
 
 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"

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);