[PATCH] D18551: Added Fixer implementation and fix() interface in clang-format for removing redundant code.

[Eric Liu via cfe-commits]

ioeric added inline comments.

================
Comment at: lib/Format/Format.cpp:1597
@@ +1596,3 @@
+                 SmallVector<CharSourceRange, 8>(Ranges.begin(), Ranges.end())),
+        UnwrappedLines(1),
+        Encoding(encoding::detectEncoding(SourceMgr.getBufferData(ID))),
----------------
djasper wrote:
> Why are you doing this?
I am not quite sure here actually. This was copied from Formatter. And since `consumeUnwrappedLine` requires at lease one element in `UnwrappedLines`, I figured this is necessary initialization?

================
Comment at: lib/Format/Format.cpp:1818
@@ +1817,3 @@
+
+    // Merge multiple continuous token deletions into one big deletion.
+    unsigned Idx = 0;
----------------
djasper wrote:
> Can you explain why you are doing this?
I want to reduce the number of replacements so that when we do `reformat`  on the fixed code, there could be fewer changed code ranges considering that the current implementation of `computeAffectedLines` is not that efficient when we have many ranges.

================
Comment at: lib/Format/Format.cpp:2286
@@ +2285,3 @@
+    const FormatStyle &, StringRef, std::vector<tooling::Range>, StringRef)>
+    ReplacementsProcessFunctionType;
+
----------------
djasper wrote:
> Don't define a typedef for something that is only used once. Also, this is an internal function, how about writing this as:
> 
>   template <typename T>
>   static tooling::Replacements
>   processReplacements(StringRef Code, const tooling::Replacements &Replaces, T ProcessFunc,
>                       const FormatStyle &Style) {
> 
>   }
> 
> No need to spell out the function type (I think).
Wow, this is really helpful! Thanks!

================
Comment at: lib/Format/Format.cpp:1590
@@ +1589,3 @@
+
+class CodeProcesser : public UnwrappedLineConsumer {
+public:
----------------
I'm not quite sure if the name is accurate. Do you have a better name?


http://reviews.llvm.org/D18551