[clang-format] Remove special handling of comments after brace/paren

[HazardyKnusperkeks]

Fixes http://llvm.org/PR55487 (#55487)

The code did not match the documentation about Cpp11BracedListStyle. Changed handling of comments after opening braces, which are supposedly function call like to behave exactly like their parenthesis counter part.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-format

Author: Björn Schäpers (HazardyKnusperkeks)

Changes

Fixes http://llvm.org/PR55487

The code did not match the documentation about Cpp11BracedListStyle. Changed handling of comments after opening braces, which are supposedly function call like to behave exactly like their parenthesis counter part.

---

Full diff: https://github.com/llvm/llvm-project/pull/71672.diff

4 Files Affected:

• (modified) clang/lib/Format/ContinuationIndenter.cpp (+3-1)
• (modified) clang/lib/Format/TokenAnnotator.cpp (+3-5)
• (modified) clang/unittests/Format/FormatTest.cpp (+11-13)
• (modified) clang/unittests/Format/FormatTestComments.cpp (+23-6)

diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index 3a829cdedb77fc7..1d76dbe39b00eae 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -802,7 +802,9 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
       Previous.isNot(TT_ObjCMethodExpr) && Previous.isNot(TT_RequiresClause) &&
       !(Current.MacroParent && Previous.MacroParent) &&
       (Current.isNot(TT_LineComment) ||
-       Previous.isOneOf(BK_BracedInit, TT_VerilogMultiLineListLParen))) {
+       (Previous.is(BK_BracedInit) &&
+        (!Previous.Previous || Previous.Previous->isNot(tok::identifier))) ||
+       Previous.is(TT_VerilogMultiLineListLParen))) {
     CurrentState.Indent = State.Column + Spaces;
     CurrentState.IsAligned = true;
   }
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 729e7e370bf62ea..9691d879735d3aa 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -3510,12 +3510,10 @@ void TokenAnnotator::calculateFormattingInformation(AnnotatedLine &Line) const {
   while (Current) {
     const FormatToken *Prev = Current->Previous;
     if (Current->is(TT_LineComment)) {
-      if (Prev->is(BK_BracedInit) && Prev->opensScope()) {
+      /*if (Prev->is(BK_BracedInit) && Prev->opensScope()) {
         Current->SpacesRequiredBefore =
-            (Style.Cpp11BracedListStyle && !Style.SpacesInParensOptions.Other)
-                ? 0
-                : 1;
-      } else if (Prev->is(TT_VerilogMultiLineListLParen)) {
+            Style.SpacesInParensOptions.Other ? 1 : 0;
+      } else */if (Prev->is(TT_VerilogMultiLineListLParen)) {
         Current->SpacesRequiredBefore = 0;
       } else {
         Current->SpacesRequiredBefore = Style.SpacesBeforeTrailingComments;
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 80903e7630c8073..e47c27c6b728c80 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -13500,20 +13500,18 @@ TEST_F(FormatTest, LayoutCxx11BraceInitializers) {
                "                                CDDDP83848_RBR_REGISTER};",
                NoBinPacking);
 
-  // FIXME: The alignment of these trailing comments might be bad. Then again,
-  // this might be utterly useless in real code.
   verifyFormat("Constructor::Constructor()\n"
-               "    : some_value{         //\n"
-               "                 aaaaaaa, //\n"
-               "                 bbbbbbb} {}");
+               "    : some_value{  //\n"
+               "          aaaaaaa, //\n"
+               "          bbbbbbb} {}");
 
   // In braced lists, the first comment is always assumed to belong to the
   // first element. Thus, it can be moved to the next or previous line as
   // appropriate.
-  verifyFormat("function({// First element:\n"
-               "          1,\n"
-               "          // Second element:\n"
-               "          2});",
+  verifyFormat("function({ // First element:\n"
+               "           1,\n"
+               "           // Second element:\n"
+               "           2});",
                "function({\n"
                "    // First element:\n"
                "    1,\n"
@@ -13582,9 +13580,9 @@ TEST_F(FormatTest, LayoutCxx11BraceInitializers) {
   verifyFormat(
       "someFunction(OtherParam,\n"
       "             BracedList{ // comment 1 (Forcing interesting break)\n"
-      "                         param1, param2,\n"
-      "                         // comment 2\n"
-      "                         param3, param4 });",
+      "                 param1, param2,\n"
+      "                 // comment 2\n"
+      "                 param3, param4 });",
       ExtraSpaces);
   verifyFormat(
       "std::this_thread::sleep_for(\n"
@@ -13635,7 +13633,7 @@ TEST_F(FormatTest, LayoutCxx11BraceInitializers) {
   verifyFormat("vector< int > x{ 1, 2, 3, 4 };", SpaceBetweenBraces);
   verifyFormat("f( {}, { {}, {} }, MyMap[ { k, v } ] );", SpaceBetweenBraces);
   verifyFormat("vector< int > x{ // comment 1\n"
-               "                 1, 2, 3, 4 };",
+               "    1, 2, 3, 4 };",
                SpaceBetweenBraces);
   SpaceBetweenBraces.ColumnLimit = 20;
   verifyFormat("vector< int > x{\n"
diff --git a/clang/unittests/Format/FormatTestComments.cpp b/clang/unittests/Format/FormatTestComments.cpp
index 967ffa32db79c75..d971744a108b531 100644
--- a/clang/unittests/Format/FormatTestComments.cpp
+++ b/clang/unittests/Format/FormatTestComments.cpp
@@ -1376,12 +1376,12 @@ TEST_F(FormatTestComments, CommentsInStaticInitializers) {
   verifyFormat("S s = {{a, b, c},  // Group #1\n"
                "       {d, e, f},  // Group #2\n"
                "       {g, h, i}}; // Group #3");
-  verifyFormat("S s = {{// Group #1\n"
-               "        a, b, c},\n"
-               "       {// Group #2\n"
-               "        d, e, f},\n"
-               "       {// Group #3\n"
-               "        g, h, i}};");
+  verifyFormat("S s = {{ // Group #1\n"
+               "         a, b, c},\n"
+               "       { // Group #2\n"
+               "         d, e, f},\n"
+               "       { // Group #3\n"
+               "         g, h, i}};");
 
   EXPECT_EQ("S s = {\n"
             "    // Some comment\n"
@@ -4576,6 +4576,23 @@ TEST_F(FormatTestComments, SplitCommentIntroducers) {
                    getLLVMStyleWithColumns(10)));
 }
 
+TEST_F(FormatTestComments, LineCommentsOnStartOfFunctionCall) {
+  verifyFormat("T foo( // Comment\n"
+               "    arg);");
+
+  verifyFormat("T bar{ // Comment\n"
+               "    arg};");
+
+  verifyFormat("T baz({ // Comment\n"
+               "        arg});");
+
+  verifyFormat("func( // Comment\n"
+               "    arg);");
+
+  verifyFormat("func({ // Comment\n"
+               "       arg});");
+}
+
 } // end namespace
 } // namespace test
 } // end namespace format

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[HazardyKnusperkeks]

Updated implementation and tests for Cpp1BracedListSyle set to false.

[HazardyKnusperkeks]

Currently containing the content of #161021, I will update once that's merged. But that should not hinder the review.

[owenca]

Now I just realized that we should add an option for the new behavior (except questionable test cases). It seems that 5a61139 only affects the formatting for Cpp11BracedListStyle: true, so we probably can change that option to an enum:

• Style1 (same as true before)

SomeFunction({// Comment 1
              "first entry",
              // Comment 2
              "second entry"});

• Style2 (space between { and the leading comment)

SomeFunction({ // Comment 1
               "first entry",
               // Comment 2
               "second entry"});

or

SomeFunction({ // Comment 1
    "first entry",
    // Comment 2
    "second entry"});

• Style3 (same as false before)

SomeFunction({ // Comment 1
               "first entry",
               // Comment 2
               "second entry" });

@mydeveloperday WDYT?

[HazardyKnusperkeks]

Not completely done, I'll have to remove the space and revert test changes. Just pushing for comments, especially the name. @owenca any ideas?

[HazardyKnusperkeks]

Now everything left is the name, maybe an adapted commit message and a review.

[owenca]

Is #89956 relevant?

[owenca]

Interestingly enough, the bug reported by the author of #55487 was already fixed somehow without this patch. (See #55487 (comment).) However, the code snippet in #55487 (comment) remains to be addressed.

[owenca]

There are a couple of places in the documentation that mention Cpp11BracedListStyle being true, which should be updated (probably to AlignFirstComment).