[PATCH] clang-apply-replacements: Add code-formatting post processing step

[Daniel Jasper]

================
Comment at: clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h:129
@@ +128,3 @@
+reformatRanges(const StringRef FileName, const RangeVector &Ranges,
+               clang::SourceManager &SM,
+               const clang::format::FormatStyle &Style);
----------------
Ranges are SourceManager-independent, so you shouldn't need to pass it in here.

Also, it seems like this is more or less a reproduction of

  tooling::Replacements reformat(const FormatStyle &Style, StringRef Code,
                                 std::vector<tooling::Range> Ranges,
                                 StringRef FileName = "<stdin>");

Defined in include/clang/Format/Format.h. Why not just use that?

================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:37
@@ -35,1 +36,3 @@
 
+static cl::opt<std::string> FormatStyleOpt(
+    "format",
----------------
I wonder whether we can/should reuse the option from tools/clang-format/ClangFormat.cpp. Seems bad to duplicate it (especially the description, which has changed repeatedly). Also, I think that "style" might be a slightly better name.

================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:61
@@ +60,3 @@
+///          couldn't be parsed.
+static bool handleFormatStyle(const char *ProgName, const StringRef OptionValue,
+                              format::FormatStyle &Style,
----------------
Seems like we should pull out the getStyle()-function out of tools/clang-format/ClangFormat.cpp and reuse it here.

================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:174
@@ +173,3 @@
+bool applyFormatting(const std::vector<tooling::Replacement> &Replacements,
+                     std::string &FileData,
+                     const format::FormatStyle &FormatStyle,
----------------
Use a separate input and output parameter, i.e.:
  StringRef InputData, std::string &OutputData,

================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:192-197
@@ +191,8 @@
+
+  if (FormattingReplacements.empty())
+    return true;
+
+  Rewriter Rewrites(SM, LangOptions());
+  if (!tooling::applyAllReplacements(FormattingReplacements, Rewrites))
+    return false;
+
----------------
AFAICT, these lines are shared between applyFormatting and applyReplacements. How about moving them into getNewDataFor (possibly renaming it to getRewrittenData).

================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:212
@@ -60,1 +211,3 @@
 
+  // Handle the -format option if present.
+  format::FormatStyle FormatStyle;
----------------
I think the option "-format" should be a bool option and there should be an additional option "-style". That makes it easier to choose and change defaults.

================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:255
@@ +254,3 @@
+    if (!applyReplacements(I->getValue(), NewFileData, Diagnostics)) {
+      errs() << "Failed to apply replacements to " << I->getKey() << "\n";
+      continue;
----------------
This should be done via the 'Diagnostics' (and also inside applyReplacements).

================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:262
@@ +261,3 @@
+                                     Diagnostics)) {
+      errs() << "Failed to apply reformatting replacements for " << I->getKey()
+             << "\n";
----------------
Same here.

================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:267
@@ +266,3 @@
+
+  coalesceRanges(ChangedRanges);
+
----------------
I think this steps adds unnecessary complexity. Ranges will in most cases be separated and even if not, clang-format should run fine with overlapping ranges. If anything, it would make more sense to move this functionality into clang-format itself as an optimization, but I doubt that it adds value.

================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:261
@@ +260,3 @@
+    const tooling::Replacement &R = *I;
+    unsigned Offset = tooling::shiftedCodePosition(Replaces, R.getOffset());
+    unsigned Length = R.getReplacementText().size();
----------------
AFAICT shiftedCodePosition does not work if the vector is not sorted (that should probably go in the comment there). Also note that implemented like this, the runtime is quadratic in the number of replacements. If we ever expect many replacements, we should probably inline the functionality of shiftedCodePosition here and shift all ranges in a single run through the loop.


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