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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
