[PATCH] Adding Replacement serialization support

[Manuel Klimek]

================
Comment at: include/clang/Tooling/ReplacementsYaml.h:28
@@ +27,3 @@
+/// \brief The top-level YAML document that contains all Replacements.
+struct ReplacementsDocument {
+  /// A freeform chunk of text to describe the context of the replacements in
----------------
I'd vote against calling this "Document". To me, a document is something I edit :)
Perhaps we can just call it yaml::Replacement? or YAMLReplacement?

================
Comment at: include/clang/Tooling/ReplacementsYaml.h:31
@@ +30,3 @@
+  /// this document.
+  std::string Context;
+
----------------
I'd put some stronger idea of what we want here into the comment:
// A chunk of text that gives context on how the replacement was generated (for example,
// the translation unit parsed, the location of a template instantiation, etc).
// This will be used for diagnostic purposes if the application of a replacement fails.

Furthermore, why don't we have one of those per replacement? If it's useful enough, I think we should really put it into the Replacement class as an optional field, and provide nice libs to populate it from a clang-tool.

Also, I'd perhaps call it DiagnosticInfo (but I don't really feel strongly here, just throwing out an idea).

================
Comment at: include/clang/Tooling/ReplacementsYaml.h:60
@@ +59,3 @@
+
+/// \brief Specialized MappingTraits for Repleacements to be converted to/from
+/// a YAML File.
----------------
*Replacements.

================
Comment at: unittests/Tooling/ReplacementsYamlTest.cpp:33-40
@@ +32,10 @@
+
+  const std::string TargetFile = "/path/to/common.h";
+  const std::string Context = "/path/to/source.cpp";
+  const unsigned int ReplacementOffset1 = 232;
+  const unsigned int ReplacementLength1 = 56;
+  const std::string ReplacementText1 = "(auto & elem : V)";
+  const unsigned int ReplacementOffset2 = 301;
+  const unsigned int ReplacementLength2 = 2;
+  const std::string ReplacementText2 = "elem";
+
----------------
In a test, I find it makes the test less readable to pull out those constants. The test will break if they don't match anyway, so I also don't think they probide any additional safety.

================
Comment at: unittests/Tooling/ReplacementsYamlTest.cpp:44
@@ +43,3 @@
+  Doc.Replacements.push_back(Replacement(TargetFile, ReplacementOffset1,
+                                        ReplacementLength1, ReplacementText1));
+  Doc.Replacements.push_back(Replacement(TargetFile, ReplacementOffset2,
----------------
Your indents are off - clang-format ;)

================
Comment at: unittests/Tooling/ReplacementsYamlTest.cpp:57
@@ +56,3 @@
+    YAML << Doc;
+    YamlContentStream.str();
+    ASSERT_NE(YamlContent.length(), 0u);
----------------
I'd use flush instead, which makes the intend clearer (if this is not intended to flush, please comment why it's there)

================
Comment at: unittests/Tooling/ReplacementsYamlTest.cpp:88-91
@@ +87,6 @@
+  ReplacementsDocument Doc;
+  Doc.Replacements.push_back(Replacement(/*FilePath=*/"target_file.h",
+                                         /*Offset=*/1,
+                                         /*Length=*/10,
+                                         /*ReplacementText=*/"replacement"));
+  std::string YamlContent;
----------------
Just a style opinion: here, too, I feel like the comments don't really give any additional benefit (as you already put descriptive values into the strings). Thus, I'd personally remove them. I don't feel strongly though.

================
Comment at: unittests/Tooling/ReplacementsYamlTest.cpp:32
@@ +31,3 @@
+TEST(ReplacementsYamlTest, writeReadTest) {
+
+  const std::string TargetFile = "/path/to/common.h";
----------------
I'd remove those empty lines at the start of tests (or at least be consistent).


http://llvm-reviews.chandlerc.com/D1422