================
Comment at: unittests/Tooling/ReplacementsYamlTest.cpp:31
@@ +30,3 @@
+
+TEST(ReplacementsYamlTest, writeReadTest) {
+
----------------
Most importantly: I think it is a bad practice to solely have a round-trip
test. You could easily mix up two parameters (say offset and length) and the
round-trip test would still pass (just the YAML files wouldn't make sense).
Could we extend the tests below to verify the contents of the generated file
and the parsed document, respectively?
nit: The second part of such a definition usually:
- Does not include "test"
- Starts upper case
- Is written as a third person verb (although this is not really consistent)
Also, "writeRead" does not really mean anything to me. How about something
along the lines of "SuccessfullyRoundTrips"?
================
Comment at: unittests/Tooling/ReplacementsYamlTest.cpp:100
@@ +99,3 @@
+ ASSERT_NE(YamlContent.length(), 0u);
+ ASSERT_EQ(std::string::npos, YamlContent.find("Context:"));
+}
----------------
Would it be hard to actually verify the entire contents?
================
Comment at: unittests/Tooling/ReplacementsYamlTest.cpp:115
@@ +114,3 @@
+ YAML >> DocActual;
+ ASSERT_NO_ERROR(YAML.error());
+}
----------------
(see first comment): Verify the contents of the parsed document.
================
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 also think that this should not be called document, but for a very different
reason:
I think this is more generic and describes a "group" or "unit" (not saying that
that would be a good name) of replacements. We need that for other things, e.g.
to apply a group of replacements atomically if some of the replacements have
conflicts with other changes.
Thus, I'd rename this (to open bikeshedding: ReplacementGroup) and move it to
Refactoring.h.
http://llvm-reviews.chandlerc.com/D1422
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
