"Escape" certain characters

tools/gn/visual_studio_writer.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "tools/gn/visual_studio_writer.h"
 
 #include <algorithm>
 #include <iterator>
 #include <map>
 #include <memory>
@@ skipping 19 matching lines @@
 #include "tools/gn/variables.h"
 #include "tools/gn/visual_studio_utils.h"
 #include "tools/gn/xml_element_writer.h"
 
 #if defined(OS_WIN)
 #include "base/win/registry.h"
 #endif
 
 namespace {
 
+std::string EscapeString(const std::string& value) {
+  std::string result;
+  if (value.find_first_of("<>&\"\t\r\n") != std::string::npos) {

[brettw, 2017/04/07 18:16:43]

I'd delete this check and always do the loop below. find_first_of will basically
do the same thing your loop+switch does and you code copies the strings in all
cases anyway, so this extra check is just extra code and runtime work.

[kylix_rd, 2017/04/07 19:18:53]

Done.

+    for (std::string::const_iterator it = value.cbegin();

[brettw, 2017/04/07 18:16:43]

I would have used a range-based for loop here which is a lot easier to read:

  for (char c : value) {
    switch (c) {
      ...
    }

[kylix_rd, 2017/04/07 19:18:53]

Yes, that's better.
Done.

+         it != value.cend();
+         ++it) {
+      switch (*it) {
+        case '\n': result += "
"; break;

[brucedawson, 2017/04/05 18:51:39]

Do you have references for the \n, \r, and \t sequences? This document suggests
that there are only five legal escape sequences in XML:

http://stackoverflow.com/questions/1091945/what-characters-do-i-need-to-escap...

Speaking of which, you might as well support '.

[kylix_rd, 2017/04/05 18:55:02]

I based it off the code in libxml here:
https://cs.chromium.org/chromium/src/third_party/libxml/src/xmlsave.c?l=2049

+        case '\r': result += "
"; break;
+        case '\t': result += "	"; break;
+        case '"': result += """; break;
+        case '<': result += "&lt;"; break;
+        case '>': result += "&gt;"; break;
+        case '&': result += "&amp;"; break;
+        default:
+          result += *it;
+      }
+    }
+  } else

[brettw, 2017/04/07 18:16:43]

Google style says all arms in an if statement should match so this should have a
{ } (although I suggest deleting it above so this is just an FYI).

+    result = value;
+  return result;
+}
+
 struct SemicolonSeparatedWriter {
   void operator()(const std::string& value, std::ostream& out) const {
-    out << value + ';';
+    out << EscapeString(value) + ';';
   }
 };
 
 struct IncludeDirWriter {
   explicit IncludeDirWriter(PathOutput& path_output)
       : path_output_(path_output) {}
   ~IncludeDirWriter() = default;
 
   void operator()(const SourceDir& dir, std::ostream& out) const {
     path_output_.WriteDir(out, dir, PathOutput::DIR_NO_LAST_SLASH);
@@ skipping 814 matching lines @@
   }
 }
 
 std::string VisualStudioWriter::GetNinjaTarget(const Target* target) {
   std::ostringstream ninja_target_out;
   DCHECK(!target->dependency_output_file().value().empty());
   ninja_path_output_.WriteFile(ninja_target_out,
                                target->dependency_output_file());
   return ninja_target_out.str();
 }