[PATCH] D17704: Added interfaces for code-formatting while applying replacements.

[Manuel Klimek via cfe-commits]

klimek added inline comments.

================
Comment at: include/clang/Tooling/Core/Replacement.h:227-232
@@ -222,1 +226,8 @@
 
+/// \brief Applies all replacements in \p Replaces to \p Code.
+///
+/// This completely ignores the path stored in each replacement. If one or more
+/// replacements cannot be applied, this returns an empty \c string.
+std::string applyAllReplacements(StringRef Code,
+                                 const std::vector<Replacements> &Replaces);
+
----------------
I think we'll need to mainly call out the differences to the function above in the comment.

================
Comment at: include/clang/Tooling/Core/Replacement.h:241-243
@@ +240,5 @@
+/// replacements.
+tooling::Replacements formatReplacements(const format::FormatStyle &Style,
+                                         StringRef Code,
+                                         const tooling::Replacements &Replaces);
+
----------------
I'd probably put the Style parameter last.

================
Comment at: lib/Tooling/Core/Replacement.cpp:14-16
@@ -13,3 +13,5 @@
 
+#include "clang/Tooling/Core/Replacement.h"
+#include <iterator>
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticIDs.h"
----------------
Nit: I'd put newlines between the #include groups here.

================
Comment at: lib/Tooling/Core/Replacement.cpp:292
@@ +291,3 @@
+  std::vector<tooling::Range> ChangedRanges = calculateChangedRanges(Replaces);
+  StringRef FileName = Replaces.begin()->getFilePath();
+  tooling::Replacements FormatReplaces =
----------------
We'll either want to assert that !Replaces.empty() (and document that in the function comment), or do an early exit if that's the case.

================
Comment at: lib/Tooling/Core/Replacement.cpp:304
@@ +303,3 @@
+
+  // Generate the new ranges from the replacements.
+  int Shift = 0;
----------------
This comment doesn't carry it's weight, I think. I'd delete it.

================
Comment at: lib/Tooling/Core/Replacement.cpp:315-317
@@ +314,5 @@
+
+bool applyAllReplacementsAndFormat(const format::FormatStyle &Style,
+                                          const Replacements &Replaces,
+                                          Rewriter &Rewrite) {
+  SourceManager &SM = Rewrite.getSourceMgr();
----------------
I think we'll want to cut that out for now and implement it so that it works for arbitrary sets of replacements in a follow-up cl.

================
Comment at: lib/Tooling/Core/Replacement.cpp:337
@@ +336,3 @@
+                                          const Replacements &Replaces) {
+  if (Replaces.empty()) return Code;
+
----------------
If the inner functions handle this case correctly, we don't need to handle it here.

================
Comment at: unittests/Tooling/RefactoringTest.cpp:171-177
@@ +170,9 @@
+TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
+  std::string Code = "MyType012345678901234567890123456789 *a =\n"
+                     "    new MyType012345678901234567890123456789();\n"
+                     "g(iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii, 0, "
+                     "jjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjj,\n"
+                     "  0, kkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk, 0, "
+                     "mmmmmmmmmmmmmmmmmmmmmmmmmmmmmmm);\n"
+                     "int  bad     = format   ;";
+  std::string Expected =
----------------
If you want to test that breaks happen, it's often better to use a configuration with a smaller column limit.


http://reviews.llvm.org/D17704