================ 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
