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

[Tareq A. Siraj]

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

================
Comment at: cpp11-migrate/Core/ReplacementsYaml.h:66
@@ +65,3 @@
+    Io.mapRequired("Length", Length);
+    Io.mapRequired("ReplacementText", ReplacementText);
+  }
----------------
Edwin Vane wrote:
> 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.
Fixed. Adding a new unittest.

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:228
@@ +227,3 @@
+                                                 ReplacementsHeaderName, Error);
+      if (!Result)
+        llvm::errs() << "Failed to generate replacements filename:" << Error
----------------
Guillaume Papin wrote:
> Don't you want to skip the rest of the code if an error occur?
Fixed.

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:234
@@ +233,3 @@
+      llvm::raw_fd_ostream ReplacementsFile(ReplacementsHeaderName.c_str(),
+                                            ErrorInfo,
+                                            llvm::sys::fs::F_Binary);
----------------
Guillaume Papin wrote:
> I believe we should start checking ErrorInfo.
> 
> The doc says:
> > If an error occurs, information about the error is put into ErrorInfo, and the stream should be immediately destroyed;
> 
Fixed.

================
Comment at: test/cpp11-migrate/HeaderReplacements/common.cpp:3
@@ +2,3 @@
+// in main.cpp
+// RUN: ls
+
----------------
Guillaume Papin wrote:
> I believe `RUN: true` will be more "idiomatic".
> Have you tried putting the file in a sub-directory? Unless they are checked recursively.
> 
> How about you rename it to something like. common.cpp.in
> Then in main.cpp:
> 
>   // RUN: cp %S/common.cpp.in %t/Test/common.cpp
Changed to "RUN: true" which is much simpler.

================
Comment at: test/cpp11-migrate/HeaderReplacements/main.cpp:9
@@ +8,3 @@
+// Check that only 1 file is generated per translation unit and header file.
+// RUN: ls %t/Test/main.cpp_common.h_*.yaml | wc -l | grep 1
+// RUN: ls %t/Test/common.cpp_common.h_*.yaml | wc -l | grep 1
----------------
Guillaume Papin wrote:
> `grep "^1$"` can avoid matching some numbers.
Fixed.

================
Comment at: test/cpp11-migrate/HeaderReplacements/main.cpp:14
@@ +13,3 @@
+// RUN: sed -i -e 's/^\(FileName:\).*[\/\\]\(.*\)$/\1 \2/g' %t/Test/common.cpp_common.h_*.yaml
+// RUN: diff --ignore-space-change %S/common.h.yaml %t/Test/main.cpp_common.h_*.yaml
+// RUN: diff --ignore-space-change %S/common.h.yaml %t/Test/common.cpp_common.h_*.yaml
----------------
Guillaume Papin wrote:
> I'm not sure how many unix tools we can use for the tests since a lot of different platforms are tested. I hope the buildbots will like all of them.
> 
> I think things like `--ignore-space-change` aren't defined in the Posix version of diff while `-b` which does the same thing apparently is.
Fixed.

================
Comment at: test/cpp11-migrate/HeaderReplacements/common.h:4
@@ +3,3 @@
+
+#include <vector>
+
----------------
Guillaume Papin wrote:
> I would suggest not using a standard include in the test, it tends to break the build on some platform.
Will change in the updated patch. Just curious, can you give me an example when this might break? I am assuming the tests are run on the same machine llvm+clang is built and they use the STL extensively.

================
Comment at: cpp11-migrate/Core/FileOverrides.h:35
@@ -33,3 +34,3 @@
 struct HeaderOverride {
-  HeaderOverride() {}
-  HeaderOverride(llvm::StringRef FileName) : FileName(FileName) {}
+public:
+  /// \brief Constructors.
----------------
Guillaume Papin wrote:
> Is `public` needed? Maybe now that HeaderOverride is not that trivial anymore it can a `class`.
Actually I meant to change it to a class, forgot :). Changed now.

================
Comment at: cpp11-migrate/Core/FileOverrides.h:38
@@ +37,3 @@
+  /// @{
+  HeaderOverride() { TransformReplacementsDoc.FileName = ""; }
+  HeaderOverride(llvm::StringRef FileName) : FileName(FileName) {
----------------
Guillaume Papin wrote:
> Is it necessary?
Removed.

================
Comment at: cpp11-migrate/Core/FileOverrides.h:46
@@ +45,3 @@
+  /// @{
+  std::string getFileName() const { return FileName; }
+  void setFileName(const std::string &S) {
----------------
Guillaume Papin wrote:
> Why not returning a StringRef instead of creating a new string?
> 
Fixed.

================
Comment at: cpp11-migrate/Core/FileOverrides.h:47
@@ +46,3 @@
+  std::string getFileName() const { return FileName; }
+  void setFileName(const std::string &S) {
+    FileName = S;
----------------
Guillaume Papin wrote:
> Why not llvm::StringRef for the argument?
Fixed.

================
Comment at: cpp11-migrate/Core/FileOverrides.h:68
@@ -37,2 +67,3 @@
+private:
   std::string FileName;
   std::string FileOverride;
----------------
Guillaume Papin wrote:
> Is the FileName required at all? Can't you just use the one in TransformDocument?
Removed.

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:105
@@ -96,1 +104,3 @@
+    HeaderOverride &HeaderOv = Headers[I->getFilePath()];
+    HeaderOv.getTransformReplacementsDoc().Replacements.push_back(TR);
   }
----------------
Guillaume Papin wrote:
> Maybe a convenience method `HeaderOverride::addXXXReplacements(StringRef TransformID, Replacement Replaces)`?
added "addTransformReplacement(...)"

================
Comment at: cpp11-migrate/Core/FileOverrides.h:64
@@ +63,3 @@
+  TransformDocument &getTransformReplacementsDoc() const {
+    return const_cast<HeaderOverride*>(this)->TransformReplacementsDoc;
+  }
----------------
Guillaume Papin wrote:
> Why not using the const_cast at the only place it is an issue, when giving the TransformDocument to YAML? Only the YAML code needs the non-const value.
Moved the const_cast to where we write to YAML.


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

BRANCH
  write_replacements_to_disk

ARCANIST PROJECT
  clang-tools-extra