[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: include/clang/Format/Format.h:769
@@ -768,1 +768,3 @@
 
+/// \brief Returns the replacements corresponding to applying and fixing
+/// \p Replaces.
----------------
I am actually not sure that fixReplacements is the right terminology here (sorry that I haven't discovered this earlier). It isn't really fixing the replacements (they aren't necessarily broken). How about cleanupAroundReplacements? Manuel, what do you think?

================
Comment at: lib/Format/Format.cpp:1596
@@ +1595,3 @@
+        RangeMgr(SourceMgr,
+                 SmallVector<CharSourceRange, 8>(Ranges.begin(), Ranges.end())),
+        UnwrappedLines(1),
----------------
But like this, it is way to much code duplication for me, both in number of lines of code as well as code complexity. Basically everything in fix() is duplicated along with most of the class members etc. The main difference seems to be that runFix() is called instead of format. There must be a way to have only one implementation of that code.

I think (but cannot see all the consequences yet) you should merge the two classes. And if that leads to too much complexity, you can probably pull out runFix and everything it calls into a separate class as well as possibly format() and everything that format calls.

================
Comment at: lib/Format/Format.cpp:1597
@@ +1596,3 @@
+                 SmallVector<CharSourceRange, 8>(Ranges.begin(), Ranges.end())),
+        UnwrappedLines(1),
+        Encoding(encoding::detectEncoding(SourceMgr.getBufferData(ID))),
----------------
Why are you doing this?

================
Comment at: lib/Format/Format.cpp:1664
@@ +1663,3 @@
+
+      if (Line.IsConstructorInitializer)
+        checkConstructorInitList(Line);
----------------
Can you move the ConstructorInitializer fixer and everything that belongs to it to a different patch? I want to look at that in more detail and two smaller patches make this easier for me.

================
Comment at: lib/Format/Format.cpp:1672
@@ +1671,3 @@
+  bool containsOnlyComments(const AnnotatedLine &Line) {
+    bool CommentsOnly = true;
+    FormatToken *Tok = Line.First;
----------------
No need for this boolean. Write this as:

  for (FormatToken *Tok = Line.First;; Tok = Tok->Next) {
    if (!Tok->is(tok::comment))
      return false;
  }
  return true;

And maybe we should (in a separate patch) add an iterator interface to AnnotatedLine so that we can use range-based for loops.

================
Comment at: lib/Format/Format.cpp:1695
@@ +1694,3 @@
+        if (NewLine == -1) {
+          IsEmpty = false;
+          continue;
----------------
Why not return -1 here?

================
Comment at: lib/Format/Format.cpp:1699
@@ +1698,3 @@
+        CurrentLine = NewLine;
+      } else if (AnnotatedLines[CurrentLine]->startsWith(tok::comment) &&
+                 containsOnlyComments(*AnnotatedLines[CurrentLine]))
----------------
Move the startsWith into containsOnlyComments.

================
Comment at: lib/Format/Format.cpp:1701
@@ +1700,3 @@
+                 containsOnlyComments(*AnnotatedLines[CurrentLine]))
+        continue;
+      else if (AnnotatedLines[CurrentLine]->startsWith(tok::r_brace))
----------------
I'd write this in a very different order:

  if (AnnotatedLines[CurrentLine]->startsWith(tok::r_brace))
    break;
  if (AnnotatedLines[CurrentLine]->startsWith(kw_namespaces) || ..) {
    int NewLine = checkEmptyNamespace(CurrentLine);
    if (NewLine == -1)
      return -1;
    continue;
  if (!containsOnlyComments(..))
    return -1;


================
Comment at: lib/Format/Format.cpp:1702
@@ +1701,3 @@
+        continue;
+      else if (AnnotatedLines[CurrentLine]->startsWith(tok::r_brace))
+        break;
----------------
It's a bit subtle that this works correctly. At least I needed to think quickly why you wouldn't falsely run into this in

  namespace N {
  void f() {
  }
  }

But of course, then the namespace wouldn't be empty. Please leave a comment explaining this briefly.

================
Comment at: lib/Format/Format.cpp:1717
@@ +1716,3 @@
+
+    for (unsigned i = InitLine; i <= CurrentLine; ++i) {
+      AnnotatedLines[i]->Deleted = true;
----------------
Hm, this seems very inelegant. If you have:

  namespace A {
  namespace B {
  namespace C {
  }
  }
  }

"namespace C {" and "}" gets delete three times. At the very least you should add

  if (AnnotatedLines[i]->Deleted)
    continue;

Thinking about this some more, it will get deleted even more often has even the outer loop in runFix will call checkEmptyNamespace again for the inner namespaces even though they have been deleted already. So inner namespaces will bei deleted O(N^2) times if N is the nesting depth. Lets get that down to 1 instead ;-)

================
Comment at: lib/Format/Format.cpp:1818
@@ +1817,3 @@
+
+    // Merge multiple continuous token deletions into one big deletion.
+    unsigned Idx = 0;
----------------
Can you explain why you are doing this?

================
Comment at: lib/Format/Format.cpp:2286
@@ +2285,3 @@
+    const FormatStyle &, StringRef, std::vector<tooling::Range>, StringRef)>
+    ReplacementsProcessFunctionType;
+
----------------
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).


http://reviews.llvm.org/D18551