Address remaining PR comments

doc/manual.asciidoc

@@ -619,12 +619,12 @@ rule cc
   command = cl /showIncludes -c $in /Fo$out
 ----
 
-3. (since Ninja 1.11) `deps = msvc_source_dependencies` specifies that the tool
-   outputs header dependencies in the form produced by Visual Studio's
-   compiler's `/sourceDependencies` flag. This flag is supported since Visual
-   Studio 16.7. Ninja will process `depfile` as a JSON file specifying header
-   dependencies immediately after the compilation finishes, then delete the
-   `.json` file (which is only used as a temporary).
+3. (since Ninja 1.11, Windows only) `deps = msvc_source_dependencies` specifies
+   that the tool outputs header dependencies in the form produced by Visual
+   Studio's compiler's `/sourceDependencies` flag. This flag is supported since
+   Visual Studio 16.7. Ninja will process `depfile` as a JSON file specifying
+   header dependencies immediately after the compilation finishes, then delete
+   the `.json` file (which is only used as a temporary).
 +
 ----
 rule cc

src/build.cc

@@ -1074,9 +1074,8 @@ bool Builder::ExtractDeps(CommandRunner::Result* result,
   } else if (deps_type == "gcc" || deps_type == "msvc_source_dependencies") {
     string depfile = result->edge->GetUnescapedDepfile();
     if (depfile.empty()) {
-      *err = string("edge with deps=");
-      err->append(deps_type);
-      err->append("but no depfile makes no sense");
+      *err = string("edge with deps=") + deps_type +
+             string(" but no depfile makes no sense");
       return false;
     }
 
@@ -1114,12 +1113,12 @@ bool Builder::ExtractDeps(CommandRunner::Result* result,
       *err = string("msvc_source_dependencies is only supported on Windows");
       return false;
 #else
-      set<string> includes;
+      std::set<std::string> includes;
       if (!ParseCLSourceDependencies(content, err, includes))
         return false;
 
-      for (set<string>::iterator i = includes.begin(); i != includes.end();
-           ++i) {
+      for (std::set<std::string>::iterator i = includes.begin();
+           i != includes.end(); ++i) {
         // Like above, ~0 is assuming that with MSVC-parsed headers, it's ok to
         // always make all backslashes.
         deps_nodes->push_back(state_->GetNode(*i, ~0u));

src/build_test.cc

@@ -1838,6 +1838,24 @@ TEST_F(BuildTest, DepsGccWithEmptyDepfileErrorsOut) {
   ASSERT_EQ(1u, command_runner_.commands_ran_.size());
 }
 
+TEST_F(BuildTest, DepsMsvcSourceDependenciesWithEmptyDepfileErrorsOut) {
+  ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
+"rule cc\n"
+"  command = cc\n"
+"  deps = msvc_source_dependencies\n"
+"build out: cc\n"));
+  Dirty("out");
+
+  string err;
+  EXPECT_TRUE(builder_.AddTarget("out", &err));
+  ASSERT_EQ("", err);
+  EXPECT_FALSE(builder_.AlreadyUpToDate());
+
+  EXPECT_FALSE(builder_.Build(&err));
+  ASSERT_EQ("subcommand failed", err);
+  ASSERT_EQ(1u, command_runner_.commands_ran_.size());
+}
+
 TEST_F(BuildTest, StatusFormatElapsed) {
   status_.BuildStarted();
   // Before any task is done, the elapsed time must be zero.

src/clsourcedependencies_parser.cc

@@ -1,3 +1,17 @@
+// Copyright (c) Microsoft Corporation.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");	#ifdef _WIN32
+// you may not use this file except in compliance with the License.	
+// You may obtain a copy of the License at	
+//
+//     http://www.apache.org/licenses/LICENSE-2.0	
+//
+// Unless required by applicable law or agreed to in writing, software	
+// distributed under the License is distributed on an "AS IS" BASIS,	
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.	
+// See the License for the specific language governing permissions and	
+// limitations under the License.
+
 #if NINJA_ENABLE_CL_SOURCE_DEPENDENCIES
 
 #include "clsourcedependencies_parser.h"
@@ -8,80 +22,69 @@
 #include "metrics.h"
 #include "util.h"
 
-using namespace rapidjson;
-
 bool ParseCLSourceDependencies(const StringPiece content, std::string* err,
                                std::set<std::string>& includes) {
   METRIC_RECORD("CLSourceDependenciesParser::Parse");
 
-  Document doc;
+  rapidjson::Document doc;
   if (doc.Parse(content.str_).HasParseError()) {
-    *err = string("sourceDependencies parse error");
+    *err = std::string("sourceDependencies parse error");
     return false;
   }
 
   if (!doc.IsObject()) {
-    *err = string("sourceDependencies root must be an object");
+    *err = std::string("sourceDependencies root must be an object");
     return false;
   }
 
   if (!doc.HasMember("Version")) {
-    *err = string("sourceDependencies missing version");
+    *err = std::string("sourceDependencies missing version");
     return false;
   }
 
-  const Value& version = doc["Version"];
+  const rapidjson::Value& version = doc["Version"];
   if (!version.IsString()) {
-    *err = string("sourceDependencies version must be a string");
+    *err = std::string("sourceDependencies version must be a string");
     return false;
   }
 
   const char* version_str = version.GetString();
   if (strncmp(version_str, "1.", 2)) {
-    *err = string("expected sourceDependencies version 1.x but found ") +
+    *err = std::string("expected sourceDependencies version 1.x but found ") +
            version_str;
     return false;
   }
 
   if (!doc.HasMember("Data")) {
-    *err = string("sourceDependencies missing data");
+    *err = std::string("sourceDependencies missing data");
     return false;
   }
 
-  const Value& data = doc["Data"];
+  const rapidjson::Value& data = doc["Data"];
   if (!data.IsObject()) {
-    *err = string("sourceDependencies data must be an object");
+    *err = std::string("sourceDependencies data must be an object");
     return false;
   }
 
   if (!data.HasMember("Includes")) {
-    *err = string("sourceDependencies data missing includes");
+    *err = std::string("sourceDependencies data missing includes");
     return false;
   }
 
-  const Value& includes_val = data["Includes"];
+  const rapidjson::Value& includes_val = data["Includes"];
   if (!includes_val.IsArray()) {
-    *err = string("sourceDependencies includes must be an array");
+    *err = std::string("sourceDependencies includes must be an array");
     return false;
   }
 
-  for (Value::ConstValueIterator i = includes_val.Begin();
+  for (rapidjson::Value::ConstValueIterator i = includes_val.Begin();
        i != includes_val.End(); ++i) {
     if (!i->IsString()) {
-      *err = string("sourceDependencies include path must be a string");
+      *err = std::string("sourceDependencies include path must be a string");
       return false;
     }
 
-    const char* include_c_str = i->GetString();
-    // JSON is encoded in UTF-8, but Ninja on Windows uses the ANSI APIs.
-    // Attempt to convert to the active code page. This could fail if the user
-    // has characters outside the code page in their paths, but there's not much
-    // we can do about that without more fundamental changes to Ninja.
-    string include_str;
-    if (!TryConvertUtf8ToAnsi(include_c_str, include_str)) {
-      *err = string("could not convert include path from UTF-8 to ANSI");
-      return false;
-    }
+    std::string include_str = std::string(i->GetString());
     if (!CLParser::IsSystemInclude(include_str))
       includes.insert(include_str);
   }

src/clsourcedependencies_parser.h

@@ -1,3 +1,17 @@
+// Copyright (c) Microsoft Corporation.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");	#ifdef _WIN32
+// you may not use this file except in compliance with the License.	
+// You may obtain a copy of the License at	
+//
+//     http://www.apache.org/licenses/LICENSE-2.0	
+//
+// Unless required by applicable law or agreed to in writing, software	
+// distributed under the License is distributed on an "AS IS" BASIS,	
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.	
+// See the License for the specific language governing permissions and	
+// limitations under the License.
+
 #if NINJA_ENABLE_CL_SOURCE_DEPENDENCIES
 
 #ifndef NINJA_CLSOURCEDEPENDENCIESPARSER_H_

src/clsourcedependencies_parser_test.cc

@@ -1,95 +1,106 @@
+// Copyright (c) Microsoft Corporation.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");	#ifdef _WIN32
+// you may not use this file except in compliance with the License.	
+// You may obtain a copy of the License at	
+//
+//     http://www.apache.org/licenses/LICENSE-2.0	
+//
+// Unless required by applicable law or agreed to in writing, software	
+// distributed under the License is distributed on an "AS IS" BASIS,	
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.	
+// See the License for the specific language governing permissions and	
+// limitations under the License.
+
 #if NINJA_ENABLE_CL_SOURCE_DEPENDENCIES
 
 #include "clsourcedependencies_parser.h"
 
 #include "test.h"
 
-// Needed for GetACP()
-#include <windows.h>
-
 TEST(ParseCLSourceDependencies, ParseInvalidJSON) {
-  string err;
-  set<string> includes;
+  std::string err;
+  std::set<std::string> includes;
 
   ASSERT_FALSE(ParseCLSourceDependencies(StringPiece("this is not JSON"), &err,
                                          includes));
   ASSERT_EQ("sourceDependencies parse error", err);
 }
 
 TEST(ParseCLSourceDependencies, ParseMissingVersion) {
-  string err;
-  set<string> includes;
+  std::string err;
+  std::set<std::string> includes;
 
   ASSERT_FALSE(ParseCLSourceDependencies(StringPiece("{}"), &err, includes));
   ASSERT_EQ("sourceDependencies missing version", err);
 }
 
 TEST(ParseCLSourceDependencies, ParseVersionWrongType) {
-  string err;
-  set<string> includes;
+  std::string err;
+  std::set<std::string> includes;
 
   ASSERT_FALSE(ParseCLSourceDependencies(StringPiece("{\"Version\":1.0}"), &err,
                                          includes));
   ASSERT_EQ("sourceDependencies version must be a string", err);
 }
 
 TEST(ParseCLSourceDependencies, ParseWrongVersion) {
-  string err;
-  set<string> includes;
+  std::string err;
+  std::set<std::string> includes;
 
   ASSERT_FALSE(ParseCLSourceDependencies(StringPiece("{\"Version\":\"2.0\"}"),
                                          &err, includes));
   ASSERT_EQ("expected sourceDependencies version 1.x but found 2.0", err);
 }
 
 TEST(ParseCLSourceDependencies, ParseMissingData) {
-  string err;
-  set<string> includes;
+  std::string err;
+  std::set<std::string> includes;
 
   ASSERT_FALSE(ParseCLSourceDependencies(StringPiece("{\"Version\":\"1.0\"}"),
                                          &err, includes));
   ASSERT_EQ("sourceDependencies missing data", err);
 }
 
 TEST(ParseCLSourceDependencies, ParseDataWrongType) {
-  string err;
-  set<string> includes;
+  std::string err;
+  std::set<std::string> includes;
 
   ASSERT_FALSE(ParseCLSourceDependencies(
       StringPiece("{\"Version\":\"1.0\",\"Data\":true}"), &err, includes));
   ASSERT_EQ("sourceDependencies data must be an object", err);
 }
 
 TEST(ParseCLSourceDependencies, ParseMissingIncludes) {
-  string err;
-  set<string> includes;
+  std::string err;
+  std::set<std::string> includes;
 
   ASSERT_FALSE(ParseCLSourceDependencies(
       StringPiece("{\"Version\":\"1.0\",\"Data\":{}}"), &err, includes));
   ASSERT_EQ("sourceDependencies data missing includes", err);
 }
 
 TEST(ParseCLSourceDependencies, ParseIncludesWrongType) {
-  string err;
-  set<string> includes;
+  std::string err;
+  std::set<std::string> includes;
 
   StringPiece content("{\"Version\":\"1.0\",\"Data\":{\"Includes\":{}}}");
   ASSERT_FALSE(ParseCLSourceDependencies(content, &err, includes));
   ASSERT_EQ("sourceDependencies includes must be an array", err);
 }
 
 TEST(ParseCLSourceDependencies, ParseBadSingleInclude) {
-  string err;
-  set<string> includes;
+  std::string err;
+  std::set<std::string> includes;
 
   StringPiece content("{\"Version\":\"1.0\",\"Data\":{\"Includes\":[23]}}");
   ASSERT_FALSE(ParseCLSourceDependencies(content, &err, includes));
   ASSERT_EQ("sourceDependencies include path must be a string", err);
 }
 
 TEST(ParseCLSourceDependencies, ParseSimple) {
-  string err;
-  set<string> includes;
+  std::string err;
+  std::set<std::string> includes;
 
   StringPiece content(
       "{\"Version\":\"1.0\",\"Data\":{\"Includes\":[\"c:\\\\test.cpp\"]}}");
@@ -98,25 +109,6 @@ TEST(ParseCLSourceDependencies, ParseSimple) {
   ASSERT_EQ("c:\\test.cpp", *includes.begin());
 }
 
-TEST(ParseCLSourceDependencies, ParseNonAscii) {
-  // This test is only designed for Windows-1252 (English default).
-  if (GetACP() != 1252)
-    return;
-
-  string content(
-      "{\"Version\":\"1.0\",\"Data\":{\"Includes\":[\"feelix\"]}}");
-  content[39] = '\xC3';
-  content[40] = '\xA9';
-
-  set<string> includes;
-  string err;
-  ASSERT_TRUE(ParseCLSourceDependencies(content, &err, includes));
-
-  const char expected[] = { '\x66', '\xE9', '\x6C', '\x69', '\x78', '\x00' };
-  ASSERT_EQ(1u, includes.size());
-  ASSERT_EQ(expected, *includes.begin());
-}
-
 TEST(ParseCLSourceDependencies, ParseReal) {
   StringPiece content(
       "{    \"Version\": \"1.0\",    \"Data\": {        \"Source\": "
@@ -373,8 +365,8 @@ TEST(ParseCLSourceDependencies, ParseReal) {
       "\"c:\\\\constants.h\",            \"c:\\\\test.h\"        ],        "
       "\"Modules\": []    }}");
 
-  string err;
-  set<string> includes;
+  std::string err;
+  std::set<std::string> includes;
   ASSERT_TRUE(ParseCLSourceDependencies(content, &err, includes));
 
   ASSERT_EQ(2u, includes.size());