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