From e74cefa6c606d69ddfd6bc9b055ef0c697c4f29a Mon Sep 17 00:00:00 2001 From: Brad King Date: Wed, 3 Feb 2016 13:44:46 -0500 Subject: Replace ManifestParser::FileReader with general FileReader Avoid having two separate filesystem interfaces. Simplify test infrastructure by avoiding custom `ManifestParser::FileReader` implementations. --- src/manifest_parser.cc | 3 +- src/manifest_parser.h | 6 +--- src/manifest_parser_perftest.cc | 10 ++----- src/manifest_parser_test.cc | 65 +++++++++++++++++------------------------ src/ninja.cc | 11 +------ 5 files changed, 32 insertions(+), 63 deletions(-) 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 #include +#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::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 files_; - vector 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); -- cgit v0.12