[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:770
@@ +769,3 @@
+/// \brief Returns the replacements corresponding to applying, fixing, and
+/// reformatting \p Replaces.
+tooling::Replacements fixReplacements(StringRef Code,
----------------
I think we should not automatically format the replacements after fixing. This function can easily be combined with formatReplacements if that is desired.

================
Comment at: include/clang/Format/Format.h:802
@@ +801,3 @@
+///
+/// Otherwise identical to the reformat() function using a file ID.
+tooling::Replacements fix(const FormatStyle &Style, StringRef Code,
----------------
This sentence doesn't make sense.

================
Comment at: include/clang/Format/Format.h:803
@@ +802,3 @@
+/// Otherwise identical to the reformat() function using a file ID.
+tooling::Replacements fix(const FormatStyle &Style, StringRef Code,
+                          ArrayRef<tooling::Range> Ranges,
----------------
I believe the style will become important as certain aspects of fixing are going to be controlled via the style. E.g. some styles might want to turn:

  if (a)
    return;

into:

  if (a) {
    return;
  }

================
Comment at: lib/Format/Format.cpp:1446
@@ -1445,1 +1445,3 @@
 
+// Returns true if 'Range' intersects with one of the input ranges.
+static bool affectsCharSourceRange(SourceManager &SourceMgr,
----------------
I'd probably move all of these out into a class so you don't need to pass the same set of parameters between all of them and so that you can make those private that aren't meant to be called directly.

================
Comment at: lib/Format/Format.cpp:1596
@@ +1595,3 @@
+
+class Fixer : public UnwrappedLineConsumer {
+public:
----------------
I am not sure, this is the right class to pull out. It still has a lot of overlap with formatter. Maybe it is instead a better idea to pull out each fixer into its own class and pull them into the formatter.

================
Comment at: lib/Format/Format.cpp:1651
@@ +1650,3 @@
+
+  tooling::Replacements fix(FormatTokenLexer &Tokens, bool *IncompleteFormat) {
+    reset();
----------------
The Tokens parameter is unused!?

================
Comment at: lib/Format/Format.cpp:1654
@@ +1653,3 @@
+
+    while (CurrentToken->isNot(tok::eof)) {
+      switch (CurrentToken->Tok.getKind()) {
----------------
I am not (yet) convinced that the fundamental design here makes sense. So far it seems to me that all fixers will either operate within a single line (ctor fixer and probably most others) or that they do something specific on multiple lines (namespace fixer and empty public/private section fixer and maybe others). Creating a new way to iterate over all tokens of all lines doesn't really add useful functionality here AFAICT.

================
Comment at: lib/Format/Format.cpp:1657
@@ +1656,3 @@
+      case tok::colon:
+        if (CurrentToken->Type == TT_CtorInitializerColon) {
+          checkConstructorInitList();
----------------
No braces. Here and at all other single-statement ifs.

================
Comment at: lib/Format/Format.cpp:1690
@@ +1689,3 @@
+
+  void reset() {
+    CurrentLine = 0;
----------------
Having a function called "reset" that is only called once is weird. Just do this in the constructor?

================
Comment at: lib/Format/Format.cpp:1748
@@ +1747,3 @@
+    unsigned Idx = 0;
+    while (Idx < Tokens.size()) {
+      unsigned St = Idx, Ed = Idx;
----------------
Comment on what this is doing.

================
Comment at: lib/Format/Format.cpp:1749
@@ +1748,3 @@
+    while (Idx < Tokens.size()) {
+      unsigned St = Idx, Ed = Idx;
+      while ((Ed + 1) < Tokens.size() && Tokens[Ed]->Next == Tokens[Ed + 1]) {
----------------
Ed? Either use E or End.

================
Comment at: lib/Format/Format.cpp:1891
@@ +1890,3 @@
+
+  struct FormatTokenLess {
+    FormatTokenLess(SourceManager &SM) : SM(SM) {}
----------------
Why is this needed?

================
Comment at: lib/Format/Format.cpp:1909
@@ +1908,3 @@
+  SmallVector<AnnotatedLine *, 16> AnnotatedLines;
+  size_t CurrentLine;
+  FormatToken DummyToken;
----------------
A lot of these need documentation. What's the current line? What is the last token (last token that was read? last token of line? last token of file?). What are RedundantTokens?

================
Comment at: lib/Format/Format.cpp:2356
@@ +2355,3 @@
+
+  std::string NewCode = applyAllReplacements(Code, Replaces);
+  std::vector<tooling::Range> ChangedRanges =
----------------
Too much code duplication. Find a way to avoid that (passing in the calls to either reformat() or fix() as lambdas to a shared internal function).

================
Comment at: unittests/Format/FormatTest.cpp:11236
@@ -11235,1 +11235,3 @@
 
+class FixTest : public ::testing::Test {
+protected:
----------------
All of this should go into a different file.


http://reviews.llvm.org/D18551