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

[Edwin Vane]

  As you guessed, this functionality is being moved from clang-modernize. First step is to add it here, next step is to remove it from clang-modernize.


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:324
@@ +323,3 @@
+
+  // cleanups unecessary ranges to finish
+  coalesceRanges(ChangedRanges);
----------------
Guillaume Papin wrote:
> Maybe correct the mistakes in this comment or simply remove it since the code that follows is self-explanatory.
I've fixed this and several other comments that got copy-pasted from clang-modernizer.

================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:328-330
@@ +327,5 @@
+
+void reformatRanges(const llvm::StringRef FileName, const RangeVec &Ranges,
+                    clang::SourceManager &SM, clang::format::FormatStyle Style,
+                    std::vector<tooling::Replacement> &Replacements) {
+  const clang::FileEntry *Entry = SM.getFileManager().getFile(FileName);
----------------
Guillaume Papin wrote:
> nit: I think we can get rid of the extra qualifications `clang::` and `llvm::`.
Actually, there's this bug in doxygen that causes it not to be able to match a function definition with it's implementation if we don't fully spell out the namespaces even though there's a using declaration.  You should find that only the function prototypes use unnecessary namespaces. Other code, or file local functions should not have unnecessary names.

================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:145
@@ +144,3 @@
+  format::FormatStyle FormatStyle;
+  if (FormatStyleOpt.getNumOccurrences() > 0) {
+    if (!handleFormatStyle(argv[0], FormatStyle, Diagnostics))
----------------
Guillaume Papin wrote:
> This line is redundant with `handleFormatStyle()`'s first line.
This test is actually for a different purpose but I changed the post condition of `handleFormatStyle()` and now format option handling is self contained.

================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:213-221
@@ +212,11 @@
+    assert(Entry && "expected an existing file");
+    FileID ID = FormattingSM.translateFile(Entry);
+    const RewriteBuffer *FormattedBuffer = 0;
+    if (!ID.isInvalid())
+      FormattedBuffer = FormattingRewriter.getRewriteBufferFor(ID);
+
+    if (FormattedBuffer)
+      FormattedBuffer->write(FileStream);
+    else
+      BufferI->second.write(FileStream);
+  }
----------------
Guillaume Papin wrote:
> Eventually:
> ```
>   const RewriteBuffer *Buffer = &BufferI->second;
>   FileID ID = FormattingSM.translateFile(Entry);
> 
>   if (!ID.isInvalid())
>     Buffer = FormattingRewriter.getRewriteBufferFor(ID);
>   Buffer->write(FileStream);
> ```
This is not the same. `getRewriteBufferFor()` may return `NULL` if a buffer wasn't changed.

================
Comment at: test/clang-apply-replacements/format.cpp:3
@@ +2,3 @@
+//
+// yes.cpp requires formatting after replacements are applied. no.cpp does not.
+//
----------------
Guillaume Papin wrote:
> Is the no.cpp test really needed?
Yes. It tests for a bug that was recently found in the migrator. If a file is changed but not formatted, it should still be written to disk. I'll update the comments.


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