[PATCH] cp11-migrate: Integration with LibFormat.

[Edwin Vane]

================
Comment at: cpp11-migrate/Core/FileOverrides.h:132
@@ +131,3 @@
+  /// removed.
+  void collectReplacementData(const clang::tooling::Replacements &Replaces);
+
----------------
Can this name be more specific to what it does: adjusting changed ranges.

================
Comment at: cpp11-migrate/Core/FileOverrides.h:136
@@ +135,3 @@
+  /// file.
+  void reformatFileChanges(llvm::StringRef Filename,
+                           const clang::format::FormatStyle &Style,
----------------
I'd suggest `reformatSingleFile()`.

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:104
@@ +103,3 @@
+
+/// \brief A functor owning a replacement, that says if a Range is
+/// "eaten" by the a replacement.
----------------
The functor doesn't own the replacement.

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:107
@@ +106,3 @@
+struct RangeContains {
+  const tooling::Replacement &R;
+  RangeContains(const tooling::Replacement &R) : R(R) {}
----------------
Data members by convention go at the end of a record definition.

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:110
@@ +109,3 @@
+
+  bool operator()(tooling::Range D) const {
+    unsigned ReplacementEnd = R.getOffset() + R.getLength();
----------------
Why D? Can we use some better variable names for R and D?

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:105
@@ +104,3 @@
+/// \brief A functor owning a replacement, that says if a Range is
+/// "eaten" by the a replacement.
+struct RangeContains {
----------------
Can we be more precise? What's "eaten" mean? Let's have some post-conditions for this functor. Looking at the logic this functor might be better called "rangeContained" since we're testing if a replacement completely covers a range.

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:150
@@ +149,3 @@
+
+    FileChangesMap[R.getFilePath()].push_back(tooling::Range(Offset, Length));
+  }
----------------
So what happens when a replacement and range partially overlap? Do you coalesce ranges or just end up with two overlapping ranges?

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:172
@@ +171,3 @@
+void SourceOverrides::reformatChanges(const clang::format::FormatStyle &Style) {
+  llvm::IntrusiveRefCntPtr<clang::DiagnosticOptions> DiagOpts(
+      new DiagnosticOptions());
----------------
Any reason we're using the heap here? Since everything has scope local to this function can we just define them on the stack?

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:224
@@ -185,1 +223,3 @@
 
+  if (ReformattingStyle)
+    for (FileOverrides::const_iterator I = FileStates.begin(),
----------------
Why not use the variable TrackReplacements from above?

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:151
@@ +150,3 @@
+  // Check the reformatting style option
+  llvm::OwningPtr<format::FormatStyle> ReformattingStyle;
+  if (FormatStyleOpt.getNumOccurrences() > 0) {
----------------
Could we put this code in a helper function to help keep the clutter of main down?

================
Comment at: docs/MigratorUsage.rst:71
@@ +70,3 @@
+
+  After all transformations have been applied it's possible to reformat the
+  changes using this option. The ``string`` arguments can be either a builtin
----------------
its not it's (which is a contraction of "it is").

================
Comment at: docs/MigratorUsage.rst:73
@@ +72,3 @@
+  changes using this option. The ``string`` arguments can be either a builtin
+  style, one of: LLVM, Google, Chromium, Mozilla or a YAML configuration file.
+  ClangFormat_ can generates the configuration file for with ``clang-format
----------------
 No :. Check out [[http://www.grammar-monster.com/lessons/semicolons_in_lists.htm|semi-colon use for lists]].

================
Comment at: docs/MigratorUsage.rst:74
@@ +73,3 @@
+  style, one of: LLVM, Google, Chromium, Mozilla or a YAML configuration file.
+  ClangFormat_ can generates the configuration file for with ``clang-format
+  -dump-config``.
----------------
**generate** not **generates**.  I'd suggest a rewordering: If you want a place to start for using your own custom configuration file, ClangFormat_ can generate a file with ...

================
Comment at: docs/MigratorUsage.rst:91
@@ +90,3 @@
+    -        MyMap.rbegin();
+    +    auto I = MyMap.rbegin();
+
----------------
Could you make the change made by -format-style more explicit here (i.e. examples with and without the option used).

================
Comment at: test/cpp11-migrate/Core/reformat.cpp:19
@@ +18,3 @@
+  std::auto_ptr<int> bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;
+  // CHECK: std::unique_ptr<int> aaaaaaaa;
+  // CHECK-NEXT: std::unique_ptr<int>
----------------
Will FileCheck still work if you add whitespace infront of the the CHECK so that the code after the : lines up?


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