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