[clang] [clang-format] Don't align comments over scopes (PR #68743)

Björn Schäpers via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 24 03:50:18 PDT 2023


https://github.com/HazardyKnusperkeks updated https://github.com/llvm/llvm-project/pull/68743

>From 3e76e5a0724af4d6f24175796856c919f30547a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Sch=C3=A4pers?= <bjoern at hazardy.de>
Date: Tue, 10 Oct 2023 22:50:43 +0200
Subject: [PATCH] [clang-format] Don't align comments over scopes

We now stop aligning trailing comments on all closing braces, for
classes etc. we even check for the semicolon between the comment and the
brace.

Fixes #67906.
---
 clang/docs/ClangFormatStyleOptions.rst        |   4 +
 clang/include/clang/Format/Format.h           |   4 +
 clang/lib/Format/WhitespaceManager.cpp        |  53 +++++-
 clang/unittests/Format/FormatTestComments.cpp | 178 ++++++++++++++++--
 4 files changed, 218 insertions(+), 21 deletions(-)

diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index cfd57f5fa8153f4..5f369ea9759edad 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -980,6 +980,10 @@ the configuration (without a prefix: ``Auto``).
 **AlignTrailingComments** (``TrailingCommentsAlignmentStyle``) :versionbadge:`clang-format 3.7` :ref:`¶ <AlignTrailingComments>`
   Control of trailing comments.
 
+  The alignment stops at closing braces after a line break, and only
+  followed by other closing braces, a (``do-``) ``while``, a lambda call, or
+  a semicolon.
+
 
   .. note::
 
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 4c344135d25163c..d0145fa9272cdd6 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -539,6 +539,10 @@ struct FormatStyle {
 
   /// Control of trailing comments.
   ///
+  /// The alignment stops at closing braces after a line break, and only
+  /// followed by other closing braces, a (``do-``) ``while``, a lambda call, or
+  /// a semicolon.
+  ///
   /// \note
   ///  As of clang-format 16 this option is not a bool but can be set
   ///  to the options. Conventional bool options still can be parsed as before.
diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp
index dbe6175fb9653ed..ff8b1e6e13a3f77 100644
--- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -1048,6 +1048,9 @@ void WhitespaceManager::alignChainedConditionals() {
 }
 
 void WhitespaceManager::alignTrailingComments() {
+  if (Style.AlignTrailingComments.Kind == FormatStyle::TCAS_Never)
+    return;
+
   const int Size = Changes.size();
   int MinColumn = 0;
   int StartOfSequence = 0;
@@ -1118,16 +1121,48 @@ void WhitespaceManager::alignTrailingComments() {
       }
     }
 
-    // We don't want to align namespace end comments.
-    const bool DontAlignThisComment =
-        I > 0 && C.NewlinesBefore == 0 &&
-        Changes[I - 1].Tok->is(TT_NamespaceRBrace);
-    if (Style.AlignTrailingComments.Kind == FormatStyle::TCAS_Never ||
-        DontAlignThisComment) {
+    // We don't want to align comments which end a scope, which are here
+    // identified by most closing braces.
+    auto DontAlignThisComment = [](const auto *Tok) {
+      if (Tok->is(tok::semi)) {
+        Tok = Tok->getPreviousNonComment();
+        if (!Tok)
+          return false;
+      }
+      if (Tok->is(tok::r_paren)) {
+        // Back up past the parentheses and a `TT_DoWhile` that may precede.
+        Tok = Tok->MatchingParen;
+        if (!Tok)
+          return false;
+        Tok = Tok->getPreviousNonComment();
+        if (!Tok)
+          return false;
+        if (Tok->is(TT_DoWhile)) {
+          const auto *Prev = Tok->getPreviousNonComment();
+          if (!Prev) {
+            // A do-while-loop without braces.
+            return true;
+          }
+          Tok = Prev;
+        }
+      }
+
+      if (Tok->isNot(tok::r_brace))
+        return false;
+
+      while (Tok->Previous && Tok->Previous->is(tok::r_brace))
+        Tok = Tok->Previous;
+      return Tok->NewlinesBefore > 0;
+    };
+
+    if (I > 0 && C.NewlinesBefore == 0 &&
+        DontAlignThisComment(Changes[I - 1].Tok)) {
       alignTrailingComments(StartOfSequence, I, MinColumn);
-      MinColumn = ChangeMinColumn;
-      MaxColumn = ChangeMinColumn;
-      StartOfSequence = I;
+      // Reset to initial values, but skip this change for the next alignment
+      // pass.
+      MinColumn = 0;
+      MaxColumn = INT_MAX;
+      StartOfSequence = I + 1;
     } else if (BreakBeforeNext || Newlines > NewLineThreshold ||
                (ChangeMinColumn > MaxColumn || ChangeMaxColumn < MinColumn) ||
                // Break the comment sequence if the previous line did not end
diff --git a/clang/unittests/Format/FormatTestComments.cpp b/clang/unittests/Format/FormatTestComments.cpp
index 1198329b7b5a8f0..5e5324f01d8670a 100644
--- a/clang/unittests/Format/FormatTestComments.cpp
+++ b/clang/unittests/Format/FormatTestComments.cpp
@@ -182,7 +182,7 @@ TEST_F(FormatTestComments, UnderstandsSingleLineComments) {
                    "int   a;     // This is unrelated"));
   EXPECT_EQ("class C {\n"
             "  void f() { // This does something ..\n"
-            "  }          // awesome..\n"
+            "  } // awesome..\n"
             "\n"
             "  int a; // This is unrelated\n"
             "};",
@@ -3102,7 +3102,8 @@ TEST_F(FormatTestComments, DontAlignNamespaceComments) {
   StringRef Input = "namespace A {\n"
                     "  TESTSUITE(B) {\n"
                     "    namespace C {\n"
-                    "      namespace D {} // namespace D\n"
+                    "      namespace D { //\n"
+                    "      } // namespace D\n"
                     "      std::string Foo = Bar; // Comment\n"
                     "      std::string BazString = Baz;   // C2\n"
                     "    }          // namespace C\n"
@@ -3114,7 +3115,8 @@ TEST_F(FormatTestComments, DontAlignNamespaceComments) {
   verifyFormat("namespace A {\n"
                "  TESTSUITE(B) {\n"
                "    namespace C {\n"
-               "      namespace D {} // namespace D\n"
+               "      namespace D { //\n"
+               "      } // namespace D\n"
                "      std::string Foo = Bar;       // Comment\n"
                "      std::string BazString = Baz; // C2\n"
                "    } // namespace C\n"
@@ -3126,7 +3128,8 @@ TEST_F(FormatTestComments, DontAlignNamespaceComments) {
   verifyFormat("namespace A {\n"
                "  TESTSUITE(B) {\n"
                "    namespace C {\n"
-               "      namespace D {} // namespace D\n"
+               "      namespace D { //\n"
+               "      } // namespace D\n"
                "      std::string Foo = Bar; // Comment\n"
                "      std::string BazString = Baz; // C2\n"
                "    } // namespace C\n"
@@ -3138,7 +3141,8 @@ TEST_F(FormatTestComments, DontAlignNamespaceComments) {
   verifyFormat("namespace A {\n"
                "  TESTSUITE(B) {\n"
                "    namespace C {\n"
-               "      namespace D {} // namespace D\n"
+               "      namespace D { //\n"
+               "      } // namespace D\n"
                "      std::string Foo = Bar; // Comment\n"
                "      std::string BazString = Baz;   // C2\n"
                "    }          // namespace C\n"
@@ -3151,7 +3155,8 @@ TEST_F(FormatTestComments, DontAlignNamespaceComments) {
   verifyFormat("namespace A {\n"
                "  TESTSUITE(B) {\n"
                "    namespace C {\n"
-               "      namespace D {} // namespace D\n"
+               "      namespace D { //\n"
+               "      } // namespace D\n"
                "      std::string Foo = Bar;       // Comment\n"
                "      std::string BazString = Baz; // C2\n"
                "    } // namespace C\n"
@@ -3163,7 +3168,8 @@ TEST_F(FormatTestComments, DontAlignNamespaceComments) {
   verifyFormat("namespace A {\n"
                "  TESTSUITE(B) {\n"
                "    namespace C {\n"
-               "      namespace D {} // namespace D\n"
+               "      namespace D { //\n"
+               "      } // namespace D\n"
                "      std::string Foo = Bar; // Comment\n"
                "      std::string BazString = Baz; // C2\n"
                "    } // namespace C\n"
@@ -3175,7 +3181,8 @@ TEST_F(FormatTestComments, DontAlignNamespaceComments) {
   verifyFormat("namespace A {\n"
                "  TESTSUITE(B) {\n"
                "    namespace C {\n"
-               "      namespace D {} // namespace D\n"
+               "      namespace D { //\n"
+               "      } // namespace D\n"
                "      std::string Foo = Bar; // Comment\n"
                "      std::string BazString = Baz;   // C2\n"
                "    }          // namespace C\n"
@@ -3191,20 +3198,167 @@ TEST_F(FormatTestComments, DontAlignNamespaceComments) {
           "}\n"
           "// Comment";
 
-#if 0
-  // FIXME: The following comment is aligned with the namespace comment.
   verifyFormat("namespace A {\n"
                "  int Foo;\n"
                "  int Bar;\n"
                "} // namespace A\n"
-               " // Comment",
+               "// Comment",
                Input, Style);
-#endif
 
   Style.FixNamespaceComments = false;
   verifyFormat(Input, Style);
 }
 
+TEST_F(FormatTestComments, DontAlignOverScope) {
+  verifyFormat("if (foo) {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("if (foo) {\n"
+               "  // something\n"
+               "} else {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("if (foo) {\n"
+               "  // something\n"
+               "} else if (foo) {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("while (foo) {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("for (;;) {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("do {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} while (foo); // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("do\n"
+               "  int aLongVariable; // with comment\n"
+               "while (foo); // not aigned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("do\n"
+               "  int aLongVariable; // with comment\n"
+               "/**/ while (foo); // not aigned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("switch (foo) {\n"
+               "case 7: {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // case not aligned\n"
+               "} // switch also not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("switch (foo) {\n"
+               "default: {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // case not aligned\n"
+               "} // switch also not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("class C {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "}; // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("struct S {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "}; // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("union U {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "}; // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("enum E {\n"
+               "  aLongVariable, // with comment\n"
+               "  f              // aligned\n"
+               "}; // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("void foo() {\n"
+               "  {\n"
+               "    int aLongVariable; // with comment\n"
+               "    int f;             // aligned\n"
+               "  } // not aligned\n"
+               "  int bar;    // new align\n"
+               "  int foobar; // group\n"
+               "}");
+
+  verifyFormat("auto longLambda = [] { // comment\n"
+               "  int aLongVariable;   // with comment\n"
+               "  int f;               // aligned\n"
+               "}; // not aligned\n"
+               "int bar;                             // new align\n"
+               "int foobar;                          // group\n"
+               "auto shortLambda = [] { return 5; }; // aligned");
+
+  verifyFormat("auto longLambdaResult = [] { // comment\n"
+               "  int aLongVariable;         // with comment\n"
+               "  int f;                     // aligned\n"
+               "}(); // not aligned\n"
+               "int bar;                               // new align\n"
+               "int foobar;                            // group\n"
+               "auto shortLambda = [] { return 5; }(); // aligned");
+
+  verifyFormat(
+      "auto longLambdaResult = [](auto I, auto J) { // comment\n"
+      "  int aLongVariable;                         // with comment\n"
+      "  int f;                                     // aligned\n"
+      "}(\"Input\", 5); // not aligned\n"
+      "int bar;                                                 // new align\n"
+      "int foobar;                                              // group\n"
+      "auto shortL = [](auto I, auto J) { return 5; }(\"In\", 5); // aligned");
+
+  verifyFormat("enum E1 { V1, V2 };                    // Aligned\n"
+               "enum E2 { LongerNames, InThis, Enum }; // Comments");
+
+  verifyFormat("class C {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} /* middle comment */; // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+}
+
 TEST_F(FormatTestComments, AlignsBlockCommentDecorations) {
   EXPECT_EQ("/*\n"
             " */",



More information about the cfe-commits mailing list