[PATCH] cpp11-migrate: Write header replacements to disk

Edwin Vane edwin.vane at intel.com
Tue Jul 23 05:33:10 PDT 2013

Comment at: cpp11-migrate/Core/ReplacementsYaml.h:43
@@ +42,3 @@
+// ScalarTraits to read/write std::string objects.
+template <>
Doxygen-style comments.

Comment at: cpp11-migrate/Core/ReplacementsYaml.h:66
@@ +65,3 @@
+    Io.mapRequired("Length", Length);
+    Io.mapRequired("ReplacementText", ReplacementText);
+  }
I think the use of temp variables here will break YAML file **reading**. the mapRequired() is forming a map between the key "X" and the address of a variable. If you're providing a temp var, whatever it reads from the file will be stored in some random location in memory and never make it to the destination structure. 

It occurs to me that since you're using an existing structure we can't really change the interface of (tooling::Replacement) you might have to use [[http://llvm.org/docs/YamlIO.html#normalization|normalization]].

I suggest you add a YAML file-reading unit test. All you have to do is ensure the file parses and the resulting doc contains expected info.

Comment at: cpp11-migrate/Core/ReplacementsYaml.h:34
@@ +33,3 @@
+    unsigned int Length = R.getLength();
+    llvm::StringRef ReplacementText = R.getReplacementText();
+    Io.mapRequired("Offset", Offset);
Tareq A. Siraj wrote:
> Edwin Vane wrote:
> > I discovered this doesn't work for reading. Check out `ScalarTraits<std::string>` in https://github.com/revane/migmerge_git/blob/master/YAML.h.
> Did you mean to refactor the tooling::Replacement class to return a string instead or just changing this to std::string here?
See my comment regarding normalization.




More information about the cfe-commits mailing list