Make clang-format cleaner remove redundant commas in list and redundant colon in constructor initializer.

[ioeric]

Summary

Make clang-format cleaner remove redundant commas/colons in constructor initializer list.

[djasper]

lib/Format/Format.cpp

1821 ↗

(On Diff #55810)

Why are we restricting this to constructor initializers? I think we should directly be more generic and clean up different lists. Also, as an idea? Could we make this very generic and implement a function that analyzes a line for a specific sequence of tokens? E.g., I would assume that then the current checkConstructorInitList() could be written as:

cleanupLeft(tok::comma, tok::comma);
cleanupLeft(tok::comma, tok::l_brace);
cleanupRight(tok::colon, tok::comma);
cleanupLeft(tok::colon, tok::l_brace);

With cleanupLeft/Right meaning: Find this sequence of tokens (ignoring comments) and then clean up the left or the right side.

Not sure about the exact names of functions etc. What do you think?

[ioeric]

lib/Format/Format.cpp

1821 ↗

(On Diff #55810)

I think having a generic cleanupLeft(tok::comma, tok::comma) is a great idea; however, the other three functions seem a little too generic since they'd probably only be used in constructor initializers? E.g. an expression like condition ? something : { list } would be a false positive for cleanupLeft(tok::colon, tok::l_brace), and [{...}, {...}] might be a false positive for cleanupLeft(tok::comma, tok::l_brace). Also, it seems to me that cleanupRight(tok::colon, tok::comma) would only happen in constructor initializers?

I think a mixed solution might work as well. For example, we can run cleanupLeft(tok::comma, tok::comma) across all tokens in the code, and then for constructor initializers specifically, we handle redundant colon and trailing comma. What do you think?

As for comments, we can probably handle them after all cleanup is done.

[ioeric]

• Extended redundant comma cleanup to general lists, and change the way constructor initializer list is handled. Removed comments cleanup, leave it for a future patch.

[ioeric]

PING

[djasper]

I experimented a bit. What do you think of this?

lib/Format/Format.cpp

1822 ↗

(On Diff #56696)

You could turn this into:

for (auto &Line : AnnotatedLines) {
  if (Line->Affected) {
    cleanupRight(Line->First, tok::comma, tok::comma);
    cleanupRight(Line->First, TT_CtorInitializerColon, tok::comma);
    cleanupLeft(Line->First, tok::comma, tok::l_brace);
    cleanupLeft(Line->First, TT_CtorInitializerColon, tok::l_brace);
  }
}

1914 ↗

(On Diff #56696)

And all of this into:

// Checks pairs {start, start->next},..., {end->previous, end} and deletes one
// of the token in the pair if the left token has \p LK token kind and the
// right token has \p RK token kind. If \p DeleteLeft is true, the left token
// is deleted on match; otherwise, the right token is deleted.
template <typename LeftKind, typename RightKind>
void cleanupPair(FormatToken *Start, LeftKind LK, RightKind RK,
                 bool DeleteLeft) {
  auto NextNotDeleted = [this](const FormatToken &Tok) -> FormatToken * {
    for (auto *Res = Tok.Next; Res; Res = Res->Next)
      if (!Res->is(tok::comment) &&
          DeletedTokens.find(Res) == DeletedTokens.end())
        return Res;
    return nullptr;
  };
  for (auto *Left = Start; Left;) {
    auto *Right = NextNotDeleted(*Left);
    if (!Right)
      break;
    if (Left->is(LK) && Right->is(RK)) {
      deleteToken(DeleteLeft ? Left : Right);
      // If the right token is deleted, we should keep the left token
      // unchanged and pair it with the new right token.
      if (!DeleteLeft)
        continue;
    }
    Left = Right;
  }
}

template <typename LeftKind, typename RightKind>
void cleanupLeft(FormatToken *Start, LeftKind LK, RightKind RK) {
  cleanupPair(Start, LK, RK, /*DeleteLeft=*/true);
}

template <typename LeftKind, typename RightKind>
void cleanupRight(FormatToken *Start, LeftKind LK, RightKind RK) {
  cleanupPair(Start, LK, RK, /*DeleteLeft=*/false);
}

[ioeric]

lib/Format/Format.cpp

1822 ↗

(On Diff #56696)

Wouldn't cleanupLeft(Line->First, tok::comma, tok::l_brace); also remove the comma from std::vector<std::vector<int>> = {{...}, {...}}?

[ioeric]

lib/Format/Format.cpp

1822 ↗

(On Diff #56696)

I should've added this case into unit test, sorry...

But I think we can either handle constructor initializer's tok::l_brace specially or annotate it? The later solution can enable us to do cleanupLeft(Line->First, tok::comma, TT_CtorInitializerLBrace);.

[ioeric]

• Use reviewer's awesome templates for checkPair().
• Remove checkConstructorInitList().
• Moved InCtorInitializer context setting before checking tok::comma so that InCtorInitializer context can be set even we have syntax error ":," around ctor colon.

lib/Format/Format.cpp

1822 ↗

(On Diff #56696)

Just found out constructor initializer's commas are already annotated. Then, this can be easily fixed with cleanupLeft(Line->First, TT_CtorInitializerComma, tok::l_brace);.

1914 ↗

(On Diff #56696)

Thanks for the awesome templates!

[ioeric]

• nit: add a missing space.

[djasper]

Looks good :-).

cfe/trunk/lib/Format/Format.cpp

analyze(TokenAnnotator &Annotator,
     // To determine if some redundant code is actually introduced by
     // replacements(e.g. deletions), we need to come up with a more
     // sophisticated way of computing affected ranges.
     AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(),
                                           AnnotatedLines.end());
 
     checkEmptyNamespace(AnnotatedLines);
 
+    for (auto &Line : AnnotatedLines) {
+      if (Line->Affected) {
+        cleanupRight(Line->First, tok::comma, tok::comma);
+        cleanupRight(Line->First, TT_CtorInitializerColon, tok::comma);
+        cleanupLeft(Line->First, TT_CtorInitializerComma, tok::l_brace);
+        cleanupLeft(Line->First, TT_CtorInitializerColon, tok::l_brace);
+      }
+    }
+
     return generateFixes();
   }
 
 private:
   bool containsOnlyComments(const AnnotatedLine &Line) {
     for (FormatToken *Tok = Line.First; Tok != nullptr; Tok = Tok->Next) {
       if (Tok->isNot(tok::comment))
         return false;
bool checkEmptyNamespace(SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
 
     for (unsigned i = InitLine; i <= CurrentLine; ++i) {
       DeletedLines.insert(i);
     }
 
     return true;
   }
 
+  // Checks pairs {start, start->next},..., {end->previous, end} and deletes one
+  // of the token in the pair if the left token has \p LK token kind and the
+  // right token has \p RK token kind. If \p DeleteLeft is true, the left token
+  // is deleted on match; otherwise, the right token is deleted.
+  template <typename LeftKind, typename RightKind>
+  void cleanupPair(FormatToken *Start, LeftKind LK, RightKind RK,
+                   bool DeleteLeft) {
+    auto NextNotDeleted = [this](const FormatToken &Tok) -> FormatToken * {
+      for (auto *Res = Tok.Next; Res; Res = Res->Next)
+        if (!Res->is(tok::comment) &&
+            DeletedTokens.find(Res) == DeletedTokens.end())
+          return Res;
+      return nullptr;
+    };
+    for (auto *Left = Start; Left;) {
+      auto *Right = NextNotDeleted(*Left);
+      if (!Right)
+        break;
+      if (Left->is(LK) && Right->is(RK)) {
+        deleteToken(DeleteLeft ? Left : Right);
+        // If the right token is deleted, we should keep the left token
+        // unchanged and pair it with the new right token.
+        if (!DeleteLeft)
+          continue;
+      }
+      Left = Right;
+    }
+  }
+
+  template <typename LeftKind, typename RightKind>
+  void cleanupLeft(FormatToken *Start, LeftKind LK, RightKind RK) {
+    cleanupPair(Start, LK, RK, /*DeleteLeft=*/true);
+  }
+
+  template <typename LeftKind, typename RightKind>
+  void cleanupRight(FormatToken *Start, LeftKind LK, RightKind RK) {
+    cleanupPair(Start, LK, RK, /*DeleteLeft=*/false);
+  }
+
   // Delete the given token.
   inline void deleteToken(FormatToken *Tok) {
     if (Tok)
       DeletedTokens.insert(Tok);
   }
 
   tooling::Replacements generateFixes() {
     tooling::Replacements Fixes;

cfe/trunk/lib/Format/TokenAnnotator.cpp

if (Current.getPrecedence() == prec::Assignment &&
                (!Current.Previous || !Current.Previous->is(tok::kw_operator))) {
       Contexts.back().IsExpression = true;
     } else if (Current.isOneOf(tok::kw_return, tok::kw_throw)) {
       Contexts.back().IsExpression = true;
     } else if (Current.is(TT_TrailingReturnArrow)) {
       Contexts.back().IsExpression = false;
     } else if (Current.is(TT_LambdaArrow) || Current.is(Keywords.kw_assert)) {
       Contexts.back().IsExpression = Style.Language == FormatStyle::LK_Java;
+    } else if (Current.Previous &&
+               Current.Previous->is(TT_CtorInitializerColon)) {
+      Contexts.back().IsExpression = true;
+      Contexts.back().InCtorInitializer = true;
     } else if (Current.isOneOf(tok::r_paren, tok::greater, tok::comma)) {
       for (FormatToken *Previous = Current.Previous;
            Previous && Previous->isOneOf(tok::star, tok::amp);
            Previous = Previous->Previous)
         Previous->Type = TT_PointerOrReference;
       if (Line.MustBeDeclaration && !Contexts.front().InCtorInitializer)
         Contexts.back().IsExpression = false;
-    } else if (Current.Previous &&
-               Current.Previous->is(TT_CtorInitializerColon)) {
-      Contexts.back().IsExpression = true;
-      Contexts.back().InCtorInitializer = true;
     } else if (Current.is(tok::kw_new)) {
       Contexts.back().CanBeExpression = false;
     } else if (Current.isOneOf(tok::semi, tok::exclaim)) {
       // This should be the condition or increment in a for-loop.
       Contexts.back().IsExpression = true;
     }
   }
 

cfe/trunk/unittests/Format/CleanupTest.cpp

TEST_F(CleanupTest, EmptyNamespaceWithCommentsBreakBeforeBrace) {
   std::string Expected = "\n\n\n\n\n\n\n\n\n\n";
   std::vector<tooling::Range> Ranges(1, tooling::Range(0, Code.size()));
   FormatStyle Style = getLLVMStyle();
   Style.BraceWrapping.AfterNamespace = true;
   std::string Result = cleanup(Code, Ranges, Style);
   EXPECT_EQ(Expected, Result);
 }
 
+TEST_F(CleanupTest, CtorInitializationSimpleRedundantComma) {
+  std::string Code = "class A {\nA() : , {} };";
+  std::string Expected = "class A {\nA()  {} };";
+  std::vector<tooling::Range> Ranges;
+  Ranges.push_back(tooling::Range(17, 0));
+  Ranges.push_back(tooling::Range(19, 0));
+  std::string Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  Code = "class A {\nA() : x(1), {} };";
+  Expected = "class A {\nA() : x(1) {} };";
+  Ranges.clear();
+  Ranges.push_back(tooling::Range(23, 0));
+  Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  Code = "class A {\nA() :,,,,{} };";
+  Expected = "class A {\nA() {} };";
+  Ranges.clear();
+  Ranges.push_back(tooling::Range(15, 0));
+  Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+}
+
+TEST_F(CleanupTest, ListSimpleRedundantComma) {
+  std::string Code = "void f() { std::vector<int> v = {1,2,,,3,{4,5}}; }";
+  std::string Expected = "void f() { std::vector<int> v = {1,2,3,{4,5}}; }";
+  std::vector<tooling::Range> Ranges;
+  Ranges.push_back(tooling::Range(40, 0));
+  std::string Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  Code = "int main() { f(1,,2,3,,4);}";
+  Expected = "int main() { f(1,2,3,4);}";
+  Ranges.clear();
+  Ranges.push_back(tooling::Range(17, 0));
+  Ranges.push_back(tooling::Range(22, 0));
+  Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+}
+
+TEST_F(CleanupTest, CtorInitializationBracesInParens) {
+  std::string Code = "class A {\nA() : x({1}),, {} };";
+  std::string Expected = "class A {\nA() : x({1}) {} };";
+  std::vector<tooling::Range> Ranges;
+  Ranges.push_back(tooling::Range(24, 0));
+  Ranges.push_back(tooling::Range(26, 0));
+  std::string Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+}
+
+TEST_F(CleanupTest, RedundantCommaNotInAffectedRanges) {
+  std::string Code =
+      "class A {\nA() : x({1}), /* comment */, { int x = 0; } };";
+  std::string Expected =
+      "class A {\nA() : x({1}), /* comment */, { int x = 0; } };";
+  // Set the affected range to be "int x = 0", which does not intercept the
+  // constructor initialization list.
+  std::vector<tooling::Range> Ranges(1, tooling::Range(42, 9));
+  std::string Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  Code = "class A {\nA() : x(1), {} };";
+  Expected = "class A {\nA() : x(1), {} };";
+  // No range. Fixer should do nothing.
+  Ranges.clear();
+  Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+}
+
+// FIXME: delete comments too.
+TEST_F(CleanupTest, CtorInitializationCommentAroundCommas) {
+  // Remove redundant commas around comment.
+  std::string Code = "class A {\nA() : x({1}), /* comment */, {} };";
+  std::string Expected = "class A {\nA() : x({1}) /* comment */ {} };";
+  std::vector<tooling::Range> Ranges;
+  Ranges.push_back(tooling::Range(25, 0));
+  Ranges.push_back(tooling::Range(40, 0));
+  std::string Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  // Remove trailing comma and ignore comment.
+  Code = "class A {\nA() : x({1}), // comment\n{} };";
+  Expected = "class A {\nA() : x({1}) // comment\n{} };";
+  Ranges = std::vector<tooling::Range>(1, tooling::Range(25, 0));
+  Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  // Remove trailing comma and ignore comment.
+  Code = "class A {\nA() : x({1}), // comment\n , y(1),{} };";
+  Expected = "class A {\nA() : x({1}), // comment\n  y(1){} };";
+  Ranges = std::vector<tooling::Range>(1, tooling::Range(38, 0));
+  Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  // Remove trailing comma and ignore comment.
+  Code = "class A {\nA() : x({1}), \n/* comment */, y(1),{} };";
+  Expected = "class A {\nA() : x({1}), \n/* comment */ y(1){} };";
+  Ranges = std::vector<tooling::Range>(1, tooling::Range(40, 0));
+  Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  // Remove trailing comma and ignore comment.
+  Code = "class A {\nA() : , // comment\n y(1),{} };";
+  Expected = "class A {\nA() :  // comment\n y(1){} };";
+  Ranges = std::vector<tooling::Range>(1, tooling::Range(17, 0));
+  Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+}
+
+TEST_F(CleanupTest, CtorInitializerInNamespace) {
+  std::string Code = "namespace A {\n"
+                     "namespace B {\n" // missing r_brace
+                     "} // namespace A\n\n"
+                     "namespace C {\n"
+                     "class A { A() : x(0),, {} };\n"
+                     "inline namespace E { namespace { } }\n"
+                     "}";
+  std::string Expected = "namespace A {\n"
+                         "\n\n\nnamespace C {\n"
+                         "class A { A() : x(0) {} };\n   \n"
+                         "}";
+  std::vector<tooling::Range> Ranges(1, tooling::Range(0, Code.size()));
+  std::string Result = cleanup(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang