[PATCH] D17727: Moved applyAllReplacementsAndFormat to clangFormat to avoid cyclic dependency.

[Manuel Klimek via cfe-commits]

klimek added inline comments.

================
Comment at: include/clang/Format/Format.h:734-736
@@ -733,1 +733,5 @@
 
+/// \brief Return replacements that are merged from orginal replacements
+/// and the replacements for formatting the code after applying the orginal
+/// replacements.
+tooling::Replacements formatReplacements(StringRef Code,
----------------
Perhaps: Returns the replacements corresponding to applying and formatting 'Replaces'.


================
Comment at: include/clang/Format/Format.h:741-743
@@ +740,5 @@
+
+/// \brief In addition to applying replacements as in `applyAllReplacements` in
+/// Replacement.h, this function also reformats the changed code after applying
+/// replacements.
+///
----------------
Given that this is now in a separate file, I'd duplicate the comment from applyAllReplacements and put a "See also" at the end of the comment.

================
Comment at: include/clang/Format/Format.h:750-751
@@ +749,4 @@
+///
+/// \returns the changed code if all replacements apply and code is fixed.
+/// empty string otherwise.
+std::string applyAllReplacementsAndFormat(StringRef Code,
----------------
.... the changed code with all replacements applied and formatted, if successful. The empty string otherwise.

================
Comment at: unittests/Format/FormatTest.cpp:12
@@ +11,3 @@
+
+#include "../Tooling/RewriterTestContext.h" // FIXME: is it okay to include it?
+#include "FormatTestUtils.h"
----------------
Yes.

================
Comment at: unittests/Format/FormatTest.cpp:11172-11174
@@ -11167,1 +11171,5 @@
 
+// This class is copied from ../Tooling/RefactoringTest.cpp
+// Although we are only use it in one test case, we need it for testing features
+// to be added in the future.
+class ReplacementTest : public ::testing::Test {
----------------
I don't think it's relevant where this is copied from, as it doesn't really contain any code, and is merely a test fixture adapter, which is to be expected.


http://reviews.llvm.org/D17727