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

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
Comment at: lib/Format/Format.cpp:1677
@@ +1676,3 @@
+
+class Formatter {
+public:
----------------
I think FormatStyle in the Processor should be const and Formatter should make its own copy to modify it.

================
Comment at: lib/Format/Format.cpp:1926
@@ +1925,3 @@
+  // Returns true if the current namespace is empty.
+  bool checkEmptyNamespace(unsigned CurrentLine, unsigned &NewLine) {
+    if (!AnnotatedLines[CurrentLine]->Last->is(tok::l_brace)) {
----------------
Why are you introducing a return value that you aren't using? I'd just return the number of subsequent lines to skip.

Overall, I'd do this a bit differently:
- Let it check the namespace starting at CurrentLine
- Delete empty namespaces within
- If it is empty, delete the outer namespace
- Always return how many lines it has looked at

That way, you don't need to store DeletedLines and you don't ever look at the same line multiple times.

================
Comment at: lib/Format/Format.cpp:1927
@@ +1926,3 @@
+  bool checkEmptyNamespace(unsigned CurrentLine, unsigned &NewLine) {
+    if (!AnnotatedLines[CurrentLine]->Last->is(tok::l_brace)) {
+      NewLine = CurrentLine;
----------------
What about 

  namespace A { // really cool namespace

?

================
Comment at: lib/Format/Format.cpp:1987
@@ +1986,3 @@
+    for (unsigned Idx = 0, End = AnnotatedLines.size() - 1; Idx < End; ++Idx) {
+      AnnotatedLines[Idx]->Last->Next = AnnotatedLines[Idx + 1]->First;
+    }
----------------
I believe that this doesn't work in many circumstances. Not sure I can easily construct a test case for the namespace cleanup, but generally, AnnotatedLines are not in a strict order. The last token of a line isn't necessarily next to the first token of the next line. This happens, e.g. for nested blocks and if preprocessor lines are intermingled with other lines.

================
Comment at: lib/Format/Format.cpp:1990-1993
@@ +1989,6 @@
+
+    // Merge multiple continuous token deletions into one big deletion so that
+    // the number of replacements can be reduced. When we do `reformat` on the
+    // fixed code, there could be fewer changed code ranges, which makes
+    // computing affected lines more efficient.
+    unsigned Idx = 0;
----------------
I think this should hardly matter considering that we are probably only going to have a few instances of cleanup replacements and the range calculation is not that inefficient.

================
Comment at: lib/Format/Format.cpp:2224
@@ +2223,3 @@
+                                         const FormatStyle &Style) {
+  // We need to use lambda function here since `reformat` has default parameter
+  // `IncompleteFormat`.
----------------
Do we also need that if we spell out the ProcessFunc above? Maybe that's actually less painful compared to writing these lambdas?


http://reviews.llvm.org/D18551