factor MSVC parsing out of CLWrapper into CLParser

4 files changed, 139 insertions, 146 deletions

diff --git a/src/msvc_helper-win32.cc b/src/msvc_helper-win32.cc
index fd9b671..be2a5e0 100644
--- a/src/msvc_helper-win32.cc
+++ b/src/msvc_helper-win32.cc
@@ -39,15 +39,15 @@ string Replace(const string& input, const string& find, const string& replace) {
   return result;
 }
 
+}  // anonymous namespace
+
 string EscapeForDepfile(const string& path) {
   // Depfiles don't escape single \.
   return Replace(path, " ", "\\ ");
 }
 
-}  // anonymous namespace
-
 // static
-string CLWrapper::FilterShowIncludes(const string& line) {
+string CLParser::FilterShowIncludes(const string& line) {
   static const char kMagicPrefix[] = "Note: including file: ";
   const char* in = line.c_str();
   const char* end = in + line.size();
@@ -63,14 +63,14 @@ string CLWrapper::FilterShowIncludes(const string& line) {
 }
 
 // static
-bool CLWrapper::IsSystemInclude(const string& path) {
+bool CLParser::IsSystemInclude(const string& path) {
   // TODO: this is a heuristic, perhaps there's a better way?
   return (path.find("program files") != string::npos ||
           path.find("microsoft visual studio") != string::npos);
 }
 
 // static
-bool CLWrapper::FilterInputFilename(const string& line) {
+bool CLParser::FilterInputFilename(const string& line) {
   // TODO: other extensions, like .asm?
   return EndsWith(line, ".c") ||
       EndsWith(line, ".cc") ||
@@ -78,7 +78,42 @@ bool CLWrapper::FilterInputFilename(const string& line) {
       EndsWith(line, ".cpp");
 }
 
-int CLWrapper::Run(const string& command, string* extra_output) {
+string CLParser::Parse(const string& output) {
+  string filtered_output;
+
+  // Loop over all lines in the output to process them.
+  size_t start = 0;
+  while (start < output.size()) {
+    size_t end = output.find_first_of("\r\n", start);
+    if (end == string::npos)
+      end = output.size();
+    string line = output.substr(start, end - start);
+
+    string include = FilterShowIncludes(line);
+    if (!include.empty()) {
+      include = IncludesNormalize::Normalize(include, NULL);
+      if (!IsSystemInclude(include))
+        includes_.insert(include);
+    } else if (FilterInputFilename(line)) {
+      // Drop it.
+      // TODO: if we support compiling multiple output files in a single
+      // cl.exe invocation, we should stash the filename.
+    } else {
+      filtered_output.append(line);
+      filtered_output.append("\n");
+    }
+
+    if (end < output.size() && output[end] == '\r')
+      ++end;
+    if (end < output.size() && output[end] == '\n')
+      ++end;
+    start = end;
+  }
+
+  return filtered_output;
+}
+
+int CLWrapper::Run(const string& command, string* output) {
   SECURITY_ATTRIBUTES security_attributes = {};
   security_attributes.nLength = sizeof(SECURITY_ATTRIBUTES);
   security_attributes.bInheritHandle = TRUE;
@@ -118,8 +153,7 @@ int CLWrapper::Run(const string& command, string* extra_output) {
     Win32Fatal("CloseHandle");
   }
 
-  // Read output of the subprocess and parse it.
-  string output;
+  // Read all output of the subprocess.
   DWORD read_len = 1;
   while (read_len) {
     char buf[64 << 10];
@@ -128,44 +162,12 @@ int CLWrapper::Run(const string& command, string* extra_output) {
         GetLastError() != ERROR_BROKEN_PIPE) {
       Win32Fatal("ReadFile");
     }
-    output.append(buf, read_len);
-
-    // Loop over all lines in the output and process them.
-    for (;;) {
-      size_t ofs = output.find_first_of("\r\n");
-      if (ofs == string::npos)
-        break;
-      string line = output.substr(0, ofs);
-
-      string include = FilterShowIncludes(line);
-      if (!include.empty()) {
-        include = IncludesNormalize::Normalize(include, NULL);
-        if (!IsSystemInclude(include))
-          includes_.insert(include);
-      } else if (FilterInputFilename(line)) {
-        // Drop it.
-        // TODO: if we support compiling multiple output files in a single
-        // cl.exe invocation, we should stash the filename.
-      } else {
-        if (extra_output) {
-          extra_output->append(line);
-          extra_output->append("\n");
-        } else {
-          printf("%s\n", line.c_str());
-        }
-      }
-
-      if (ofs < output.size() && output[ofs] == '\r')
-        ++ofs;
-      if (ofs < output.size() && output[ofs] == '\n')
-        ++ofs;
-      output = output.substr(ofs);
-    }
+    output->append(buf, read_len);
   }
 
+  // Wait for it to exit and grab its exit code.
   if (WaitForSingleObject(process_info.hProcess, INFINITE) == WAIT_FAILED)
     Win32Fatal("WaitForSingleObject");
-
   DWORD exit_code = 0;
   if (!GetExitCodeProcess(process_info.hProcess, &exit_code))
     Win32Fatal("GetExitCodeProcess");
@@ -178,11 +180,3 @@ int CLWrapper::Run(const string& command, string* extra_output) {
 
   return exit_code;
 }
-
-vector<string> CLWrapper::GetEscapedResult() {
-  vector<string> result;
-  for (set<string>::iterator i = includes_.begin(); i != includes_.end(); ++i) {
-    result.push_back(EscapeForDepfile(*i));
-  }
-  return result;
-}
diff --git a/src/msvc_helper.h b/src/msvc_helper.h
index 102201b..32ab606 100644
--- a/src/msvc_helper.h
+++ b/src/msvc_helper.h
@@ -17,23 +17,13 @@
 #include <vector>
 using namespace std;
 
+string EscapeForDepfile(const string& path);
+
 /// Visual Studio's cl.exe requires some massaging to work with Ninja;
 /// for example, it emits include information on stderr in a funny
-/// format when building with /showIncludes.  This class wraps a CL
-/// process and parses that output to extract the file list.
-struct CLWrapper {
-  CLWrapper() : env_block_(NULL) {}
-
-  /// Set the environment block (as suitable for CreateProcess) to be used
-  /// by Run().
-  void SetEnvBlock(void* env_block) { env_block_ = env_block; }
-
-  /// Start a process and parse its output.  Returns its exit code.
-  /// Any non-parsed output is buffered into \a extra_output if provided,
-  /// otherwise it is printed to stdout while the process runs.
-  /// Crashes (calls Fatal()) on error.
-  int Run(const string& command, string* extra_output=NULL);
-
+/// format when building with /showIncludes.  This class parses this
+/// output.
+struct CLParser {
   /// Parse a line of cl.exe output and extract /showIncludes info.
   /// If a dependency is extracted, returns a nonempty string.
   /// Exposed for testing.
@@ -50,10 +40,24 @@ struct CLWrapper {
   /// Exposed for testing.
   static bool FilterInputFilename(const string& line);
 
-  /// Fill a vector with the unique'd headers, escaped for output as a .d
-  /// file.
-  vector<string> GetEscapedResult();
+  /// Parse the full output of cl, returning the output (if any) that
+  /// should printed.
+  string Parse(const string& output);
 
-  void* env_block_;
   set<string> includes_;
 };
+
+/// Wraps a synchronous execution of a CL subprocess.
+struct CLWrapper {
+  CLWrapper() : env_block_(NULL) {}
+
+  /// Set the environment block (as suitable for CreateProcess) to be used
+  /// by Run().
+  void SetEnvBlock(void* env_block) { env_block_ = env_block; }
+
+  /// Start a process and gather its raw output.  Returns its exit code.
+  /// Crashes (calls Fatal()) on error.
+  int Run(const string& command, string* output);
+
+  void* env_block_;
+};
diff --git a/src/msvc_helper_main-win32.cc b/src/msvc_helper_main-win32.cc
index 0bbe98b..3192821 100644
--- a/src/msvc_helper_main-win32.cc
+++ b/src/msvc_helper_main-win32.cc
@@ -44,12 +44,13 @@ void PushPathIntoEnvironment(const string& env_block) {
   }
 }
 
-void WriteDepFileOrDie(const char* object_path, CLWrapper* cl) {
+void WriteDepFileOrDie(const char* object_path, const CLParser& parse) {
   string depfile_path = string(object_path) + ".d";
   FILE* depfile = fopen(depfile_path.c_str(), "w");
   if (!depfile) {
     unlink(object_path);
-    Fatal("opening %s: %s", depfile_path.c_str(), GetLastErrorString().c_str());
+    Fatal("opening %s: %s", depfile_path.c_str(),
+          GetLastErrorString().c_str());
   }
   if (fprintf(depfile, "%s: ", object_path) < 0) {
     unlink(object_path);
@@ -57,9 +58,9 @@ void WriteDepFileOrDie(const char* object_path, CLWrapper* cl) {
     unlink(depfile_path.c_str());
     Fatal("writing %s", depfile_path.c_str());
   }
-  vector<string> headers = cl->GetEscapedResult();
-  for (vector<string>::iterator i = headers.begin(); i != headers.end(); ++i) {
-    if (fprintf(depfile, "%s\n", i->c_str()) < 0) {
+  const set<string>& headers = parse.includes_;
+  for (set<string>::iterator i = headers.begin(); i != headers.end(); ++i) {
+    if (fprintf(depfile, "%s\n", EscapeForDepfile(*i).c_str()) < 0) {
       unlink(object_path);
       fclose(depfile);
       unlink(depfile_path.c_str());
@@ -95,11 +96,6 @@ int MSVCHelperMain(int argc, char** argv) {
     }
   }
 
-  if (!output_filename) {
-    Usage();
-    Fatal("-o required");
-  }
-
   string env;
   if (envfile) {
     string err;
@@ -118,9 +114,15 @@ int MSVCHelperMain(int argc, char** argv) {
   CLWrapper cl;
   if (!env.empty())
     cl.SetEnvBlock((void*)env.data());
-  int exit_code = cl.Run(command);
+  string output;
+  int exit_code = cl.Run(command, &output);
 
-  WriteDepFileOrDie(output_filename, &cl);
+  if (output_filename) {
+    CLParser parser;
+    output = parser.Parse(output);
+    WriteDepFileOrDie(output_filename, parser);
+  }
+  printf("%s\n", output.c_str());
 
   return exit_code;
 }
diff --git a/src/msvc_helper_test.cc b/src/msvc_helper_test.cc
index 7730425..1e1cbde 100644
--- a/src/msvc_helper_test.cc
+++ b/src/msvc_helper_test.cc
@@ -19,100 +19,93 @@
 #include "test.h"
 #include "util.h"
 
-TEST(MSVCHelperTest, ShowIncludes) {
-  ASSERT_EQ("", CLWrapper::FilterShowIncludes(""));
+TEST(CLParserTest, ShowIncludes) {
+  ASSERT_EQ("", CLParser::FilterShowIncludes(""));
 
-  ASSERT_EQ("", CLWrapper::FilterShowIncludes("Sample compiler output"));
+  ASSERT_EQ("", CLParser::FilterShowIncludes("Sample compiler output"));
   ASSERT_EQ("c:\\Some Files\\foobar.h",
-            CLWrapper::FilterShowIncludes("Note: including file: "
-                                          "c:\\Some Files\\foobar.h"));
+            CLParser::FilterShowIncludes("Note: including file: "
+                                         "c:\\Some Files\\foobar.h"));
   ASSERT_EQ("c:\\initspaces.h",
-            CLWrapper::FilterShowIncludes("Note: including file:    "
-                                          "c:\\initspaces.h"));
+            CLParser::FilterShowIncludes("Note: including file:    "
+                                         "c:\\initspaces.h"));
 }
 
-TEST(MSVCHelperTest, FilterInputFilename) {
-  ASSERT_TRUE(CLWrapper::FilterInputFilename("foobar.cc"));
-  ASSERT_TRUE(CLWrapper::FilterInputFilename("foo bar.cc"));
-  ASSERT_TRUE(CLWrapper::FilterInputFilename("baz.c"));
+TEST(CLParserTest, FilterInputFilename) {
+  ASSERT_TRUE(CLParser::FilterInputFilename("foobar.cc"));
+  ASSERT_TRUE(CLParser::FilterInputFilename("foo bar.cc"));
+  ASSERT_TRUE(CLParser::FilterInputFilename("baz.c"));
 
-  ASSERT_FALSE(CLWrapper::FilterInputFilename(
+  ASSERT_FALSE(CLParser::FilterInputFilename(
                    "src\\cl_helper.cc(166) : fatal error C1075: end "
                    "of file found ..."));
 }
 
-TEST(MSVCHelperTest, Run) {
-  CLWrapper cl;
-  string output;
-  cl.Run("cmd /c \"echo foo&& echo Note: including file:  foo.h&&echo bar\"",
-         &output);
+TEST(CLParserTest, ParseSimple) {
+  CLParser parser;
+  string output = parser.Parse(
+      "foo\r\n"
+      "Note: including file:  foo.h\r\n"
+      "bar\r\n");
+
   ASSERT_EQ("foo\nbar\n", output);
-  ASSERT_EQ(1u, cl.includes_.size());
-  ASSERT_EQ("foo.h", *cl.includes_.begin());
+  ASSERT_EQ(1u, parser.includes_.size());
+  ASSERT_EQ("foo.h", *parser.includes_.begin());
 }
 
-TEST(MSVCHelperTest, RunFilenameFilter) {
-  CLWrapper cl;
-  string output;
-  cl.Run("cmd /c \"echo foo.cc&& echo cl: warning\"",
-         &output);
+TEST(CLParserTest, ParseFilenameFilter) {
+  CLParser parser;
+  string output = parser.Parse(
+      "foo.cc\r\n"
+      "cl: warning\r\n");
   ASSERT_EQ("cl: warning\n", output);
 }
 
-TEST(MSVCHelperTest, RunSystemInclude) {
-  CLWrapper cl;
-  string output;
-  cl.Run("cmd /c \"echo Note: including file: c:\\Program Files\\foo.h&&"
-         "echo Note: including file: d:\\Microsoft Visual Studio\\bar.h&&"
-         "echo Note: including file: path.h\"",
-         &output);
+TEST(CLParserTest, ParseSystemInclude) {
+  CLParser parser;
+  string output = parser.Parse(
+      "Note: including file: c:\\Program Files\\foo.h\r\n"
+      "Note: including file: d:\\Microsoft Visual Studio\\bar.h\r\n"
+      "Note: including file: path.h\r\n");
   // We should have dropped the first two includes because they look like
   // system headers.
   ASSERT_EQ("", output);
-  ASSERT_EQ(1u, cl.includes_.size());
-  ASSERT_EQ("path.h", *cl.includes_.begin());
-}
-
-TEST(MSVCHelperTest, EnvBlock) {
-  char env_block[] = "foo=bar\0";
-  CLWrapper cl;
-  cl.SetEnvBlock(env_block);
-  string output;
-  cl.Run("cmd /c \"echo foo is %foo%", &output);
-  ASSERT_EQ("foo is bar\n", output);
+  ASSERT_EQ(1u, parser.includes_.size());
+  ASSERT_EQ("path.h", *parser.includes_.begin());
 }
 
-TEST(MSVCHelperTest, DuplicatedHeader) {
-  CLWrapper cl;
-  string output;
-  cl.Run("cmd /c \"echo Note: including file: foo.h&&"
-         "echo Note: including file: bar.h&&"
-         "echo Note: including file: foo.h\"",
-         &output);
+TEST(CLParserTest, DuplicatedHeader) {
+  CLParser parser;
+  string output = parser.Parse(
+      "Note: including file: foo.h\r\n"
+      "Note: including file: bar.h\r\n"
+      "Note: including file: foo.h\r\n");
   // We should have dropped one copy of foo.h.
   ASSERT_EQ("", output);
-  ASSERT_EQ(2u, cl.includes_.size());
+  ASSERT_EQ(2u, parser.includes_.size());
 }
 
-TEST(MSVCHelperTest, DuplicatedHeaderPathConverted) {
-  CLWrapper cl;
-  string output;
-  cl.Run("cmd /c \"echo Note: including file: sub/foo.h&&"
-         "echo Note: including file: bar.h&&"
-         "echo Note: including file: sub\\foo.h\"",
-         &output);
+TEST(CLParserTest, DuplicatedHeaderPathConverted) {
+  CLParser parser;
+  string output = parser.Parse(
+      "Note: including file: sub/foo.h\r\n"
+      "Note: including file: bar.h\r\n"
+      "Note: including file: sub\\foo.h\r\n");
   // We should have dropped one copy of foo.h.
   ASSERT_EQ("", output);
-  ASSERT_EQ(2u, cl.includes_.size());
+  ASSERT_EQ(2u, parser.includes_.size());
 }
 
-TEST(MSVCHelperTest, SpacesInFilename) {
+TEST(CLParserTest, SpacesInFilename) {
+  ASSERT_EQ("sub\\some\\ sdk\\foo.h",
+            EscapeForDepfile("sub\\some sdk\\foo.h"));
+}
+
+TEST(MSVCHelperTest, EnvBlock) {
+  char env_block[] = "foo=bar\0";
   CLWrapper cl;
+  cl.SetEnvBlock(env_block);
   string output;
-  cl.Run("cmd /c \"echo Note: including file: sub\\some sdk\\foo.h",
-         &output);
-  ASSERT_EQ("", output);
-  vector<string> headers = cl.GetEscapedResult();
-  ASSERT_EQ(1u, headers.size());
-  ASSERT_EQ("sub\\some\\ sdk\\foo.h", headers[0]);
+  cl.Run("cmd /c \"echo foo is %foo%", &output);
+  ASSERT_EQ("foo is bar\r\n", output);
 }