[PATCH] cpp11-migrate: FileOverrides/Transform refactoring.

[Guillaume Papin]

================
Comment at: cpp11-migrate/Core/FileOverrides.h:48
@@ -45,2 +47,3 @@
 
 /// \brief Container storing the file content overrides for a source file.
+class SourceOverrides {
----------------
Edwin Vane wrote:
> Can you add //...for a source file and any headers included by the source file either directly or indirectly to which changes have been made.//
Sure, done.

================
Comment at: cpp11-migrate/Core/FileOverrides.h:87
@@ +86,3 @@
+
+private:
+  const std::string MainFileName;
----------------
Tareq A. Siraj wrote:
> Already under private:
I like to have the private fields clearly delimited but I guess it's just a personal matters.

Looking at Transforms.h I see the same patterns but looking clang/include/clang/Tooling/Refactoring.h I see both situations, with and without the extra `private:`. I believe there is no strong rule about this.

Anyway it's removed.

================
Comment at: cpp11-migrate/Core/FileOverrides.h:88
@@ -62,1 +87,3 @@
+private:
+  const std::string MainFileName;
   std::string MainFileOverride;
----------------
Tareq A. Siraj wrote:
> Do we need to store MainFileName? Can we use a StringRef instead.
It's a bit dangerous to store as a StringRef, it's difficult to constrain that the string passed has to outlive the SourceOverrides, which is a class that is made to live long. Furthermore no performance issues should arise from this.

================
Comment at: cpp11-migrate/Core/FileOverrides.h:102
@@ +101,3 @@
+
+public:
+  FileOverrides() {}
----------------
Tareq A. Siraj wrote:
> Already under public:
Fixed.

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:19
@@ -18,2 +18,3 @@
 #include "clang/Basic/SourceManager.h"
-#include "llvm/Support/Path.h"
+
+#include "clang/Format/Format.h"
----------------
Tareq A. Siraj wrote:
> Unnecessary empty line.
Fixed and noted, I will avoid doing this in the future.

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:153
@@ +152,3 @@
+  if (Override == NULL)
+    Override = new SourceOverrides(Filename);
+  return *Override;
----------------
Edwin Vane wrote:
> This looks like a memory leak to me. Should not this new SourceOverrides be stored in Overrides?
It is.

  SourceOverrides *&Override = Overrides[Filename];

We take a reference to the pointer `SourceOverrides *&`, so in the 'new' assignment update the referenced value (which is in the map).

================
Comment at: cpp11-migrate/Core/SyntaxCheck.h:19
@@ -18,4 +18,3 @@
 
-#include "Core/FileOverrides.h"
-
+#include <string>
 #include <vector>
----------------
Tareq A. Siraj wrote:
> Do we need this?
Yes because of `const std::vector<std::string> &SourcePaths` below. `<string>` used to be included in the removed include "Core/FileOverrides.h".

================
Comment at: cpp11-migrate/Core/SyntaxCheck.cpp:17
@@ +16,3 @@
+#include "Core/SyntaxCheck.h"
+
+#include "Core/FileOverrides.h"
----------------
Edwin Vane wrote:
> Tareq A. Siraj wrote:
> > Unnecessary empty line.
> Unneeded vertical whitespace.
Fixed

================
Comment at: cpp11-migrate/Core/Transform.h:53
@@ -53,3 +52,3 @@
 
-class RewriterManager;
+class FileOverrides;
 
----------------
Tareq A. Siraj wrote:
> Do we need this?
Yes.

================
Comment at: cpp11-migrate/Core/Transform.cpp:17
@@ -16,1 +16,3 @@
 #include "Core/Transform.h"
+
+#include "Core/FileOverrides.h"
----------------
Tareq A. Siraj wrote:
> Unnecessary empty line.
Fixed.

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:23
@@ -22,1 +22,3 @@
+#include "Core/Transforms.h"
+
 #include "LoopConvert/LoopConvert.h"
----------------
Edwin Vane wrote:
> Unnecessary blank line.
Fixed.

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:35
@@ -32,2 +34,3 @@
 namespace cl = llvm::cl;
+using namespace clang;
 using namespace clang::tooling;
----------------
Tareq A. Siraj wrote:
> Do we need this?
It wasn't in use in this commit, it's a leftover of my upcoming patch for libformat. Anyway it's used by some changes I made from this review.

================
Comment at: unittests/cpp11-migrate/FileOverridesTest.cpp:11
@@ +10,3 @@
+#include "Core/FileOverrides.h"
+
+#include "gtest/gtest.h"
----------------
Edwin Vane wrote:
> Remove extra blank lines from include statements.
Fixed.

================
Comment at: unittests/cpp11-migrate/FileOverridesTest.cpp:43
@@ +42,3 @@
+    ASSERT_NO_ERROR(
+        sys::fs::createTemporaryFile("cpp11-migrate", "cpp", FD, Path));
+
----------------
Edwin Vane wrote:
> I recommend not actually creating files on disk. Instead, create files in memory and use SourceManager.overrideFileContents() to make it use the file in memory. This means you need access to the SourceManager created with the rewriter in applyReplacements() so you'll need to think about how to make this work.
Fixed (took me more time than it should have to figure out what to do...).

================
Comment at: unittests/cpp11-migrate/IncludeExcludeTest.cpp:1
@@ +1,2 @@
+//===- cpp11-migrate/IncludeExcludeTest.cpp - IncludeExclude unit tests ---===//
+//
----------------
Edwin Vane wrote:
> Changes to this file are unrelated to your refactoring. Please put in another commit.
ok.

================
Comment at: unittests/cpp11-migrate/PerfSupportTest.cpp:1
@@ -1,1 +1,2 @@
-#include "gtest/gtest.h"
+//===- cpp11-migrate/PerfSupportTest.cpp - PerfSupport unit tests --------===//
+//
----------------
Edwin Vane wrote:
> Adding headers is unrelated to your refactoring. please put them in another commit.
ok

================
Comment at: unittests/cpp11-migrate/TransformTest.cpp:1
@@ -1,1 +1,2 @@
-#include "gtest/gtest.h"
+//===- cpp11-migrate/TransformTest.cpp - Transform unit tests -------------===//
+//
----------------
Edwin Vane wrote:
> Changes to this file are not related to your refactoring. Please put in another commit. And don't add extra blank lines between `#include`s.
ok.

================
Comment at: unittests/cpp11-migrate/TransformTest.cpp:14
@@ +13,3 @@
+
+#include "Core/FileOverrides.h"
+
----------------
Tareq A. Siraj wrote:
> Do we need this for this test?
Yes.


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