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

[Guillaume Papin]

================
Comment at: test/cpp11-migrate/HeaderReplacements/common.cpp:3
@@ +2,3 @@
+// in main.cpp
+// RUN: ls
+
----------------
If the file is moved to a sub-directory is it still needed?
Also I would use `RUN: true` or `RUN: :` instead if no other choice (but for the later I'm not sure about it's availability).

================
Comment at: test/cpp11-migrate/HeaderReplacements/main.cpp:25
@@ +24,3 @@
+// RUN: sed -i -e 's/^\(FileName:\).*[\/\\]\(.*\)$/\1 \2/g' %t/Test/main.cpp_common.h_*.yaml
+// RUN: diff --ignore-space-change %p/common.h.yaml %t/Test/main.cpp_common.h_*.yaml
+
----------------
I believe using `diff -b` is more widely supported than `diff --ignore-space-change`. The first one is specified by Posix but not the other one.

================
Comment at: test/cpp11-migrate/HeaderReplacements/main.cpp:8
@@ +7,3 @@
+// RUN: cpp11-migrate -loop-convert -headers -include=%t/Test %t/Test/main.cpp %t/Test/common.cpp --
+// 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
----------------
Edwin Vane wrote:
> Could you add a comment explaning what these commands are doing? Especially the seds.
grep 1 will match 10 and maybe it's not necessary to run ls since the shell is doing the file-expansion work, echo is enough.

What about something like: `echo %t/Test/main.cpp_common.h_*.yaml | wc -w | grep -q "^1$"`

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:234
@@ +233,3 @@
+      llvm::raw_fd_ostream ReplacementsFile(ReplacementsHeaderName.c_str(),
+                                            ErrorInfo,
+                                            llvm::sys::fs::F_Binary);
----------------
I think it would be nice to check `ErrorInfo` and not just pass it to the raw_fd_ostream constructor.
For example if the directory is not writable, I think the stream will be erroneous, and as the doc says (http://llvm.org/docs/doxygen/html/classllvm_1_1raw__fd__ostream.html#a67832b6ac28ac72c92668068ce623e10): 

>  If an error occurs, information about the error is put into ErrorInfo, and the stream should be immediately destroyed; the string will be empty if no error occurred



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

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

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


================
Comment at: cpp11-migrate/Core/FileOverrides.h:64
@@ +63,3 @@
+  TransformDocument &getTransformReplacementsDoc() const {
+    return const_cast<HeaderOverride*>(this)->TransformReplacementsDoc;
+  }
----------------
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.

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

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

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

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:234
@@ +233,3 @@
+      llvm::raw_fd_ostream ReplacementsFile(ReplacementsHeaderName.c_str(),
+                                            ErrorInfo,
+                                            llvm::sys::fs::F_Binary);
----------------
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;


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

================
Comment at: test/cpp11-migrate/HeaderReplacements/common.h:4
@@ +3,3 @@
+
+#include <vector>
+
----------------
I would suggest not using a standard include in the test, it tends to break the build on some platform.

================
Comment at: test/cpp11-migrate/HeaderReplacements/common.cpp:3
@@ +2,3 @@
+// in main.cpp
+// RUN: ls
+
----------------
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

================
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
----------------
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.

================
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
----------------
`grep "^1$"` can avoid matching some numbers.


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

BRANCH
  write_replacements_to_disk

ARCANIST PROJECT
  clang-tools-extra