[PATCH] cpp11-migrate: Write header replacements to disk
Guillaume Papin
guillaume.papin at epitech.eu
Tue Jul 23 06:34:42 PDT 2013
================
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
More information about the cfe-commits
mailing list