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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
