[clang-format] Handle variable declarations in BreakAfterAttributes

[owenca]

Also cleaned up some old test cases.

Fixes #71563.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-format

Author: Owen Pan (owenca)

Changes

Also cleaned up some old test cases.

Fixes #71563.

---

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

4 Files Affected:

• (modified) clang/docs/ClangFormatStyleOptions.rst (+2-2)
• (modified) clang/include/clang/Format/Format.h (+2-2)
• (modified) clang/lib/Format/TokenAnnotator.cpp (+18-15)
• (modified) clang/unittests/Format/FormatTest.cpp (+27-11)

diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index 21342e1b89ea866..3b9c4bcf19b2c2d 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -2049,8 +2049,8 @@ the configuration (without a prefix: ``Auto``).
 .. _BreakAfterAttributes:
 
 **BreakAfterAttributes** (``AttributeBreakingStyle``) :versionbadge:`clang-format 16` :ref:`¶ <BreakAfterAttributes>`
-  Break after a group of C++11 attributes before a function
-  declaration/definition name.
+  Break after a group of C++11 attributes before a variable/function
+  (including constructor/destructor) declaration/definition name.
 
   Possible values:
 
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 3e9d1915badd87f..8282f9206ddd2fd 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -1449,8 +1449,8 @@ struct FormatStyle {
     ABS_Never,
   };
 
-  /// Break after a group of C++11 attributes before a function
-  /// declaration/definition name.
+  /// Break after a group of C++11 attributes before a variable/function
+  /// (including constructor/destructor) declaration/definition name.
   /// \version 16
   AttributeBreakingStyle BreakAfterAttributes;
 
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 729e7e370bf62ea..210152d0846f4f9 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2000,6 +2000,10 @@ class AnnotatingParser {
                (!Line.MightBeFunctionDecl || Current.NestingLevel != 0)) {
       Contexts.back().FirstStartOfName = &Current;
       Current.setType(TT_StartOfName);
+      if (auto *PrevNonComment = Current.getPreviousNonComment();
+          PrevNonComment && PrevNonComment->is(TT_StartOfName)) {
+        PrevNonComment->setType(TT_Unknown);
+      }
     } else if (Current.is(tok::semi)) {
       // Reset FirstStartOfName after finding a semicolon so that a for loop
       // with multiple increment statements is not confused with a for loop
@@ -3258,7 +3262,7 @@ static bool isFunctionDeclarationName(bool IsCpp, const FormatToken &Current,
   if (Current.is(TT_FunctionDeclarationName))
     return true;
 
-  if (!Current.Tok.getIdentifierInfo())
+  if (!Current.Tok.getIdentifierInfo() || Current.is(TT_CtorDtorDeclName))
     return false;
 
   auto skipOperatorName = [](const FormatToken *Next) -> const FormatToken * {
@@ -3441,29 +3445,28 @@ void TokenAnnotator::calculateFormattingInformation(AnnotatedLine &Line) const {
   if (AlignArrayOfStructures)
     calculateArrayInitializerColumnList(Line);
 
+  const bool IsCpp = Style.isCpp();
   bool LineIsFunctionDeclaration = false;
   FormatToken *ClosingParen = nullptr;
   for (FormatToken *Tok = Current, *AfterLastAttribute = nullptr; Tok;
        Tok = Tok->Next) {
     if (Tok->Previous->EndsCppAttributeGroup)
       AfterLastAttribute = Tok;
-    if (const bool IsCtorOrDtor = Tok->is(TT_CtorDtorDeclName);
-        IsCtorOrDtor ||
-        isFunctionDeclarationName(Style.isCpp(), *Tok, Line, ClosingParen)) {
-      if (!IsCtorOrDtor) {
-        LineIsFunctionDeclaration = true;
-        Tok->setFinalizedType(TT_FunctionDeclarationName);
-      }
-      if (AfterLastAttribute &&
-          mustBreakAfterAttributes(*AfterLastAttribute, Style)) {
-        AfterLastAttribute->MustBreakBefore = true;
-        Line.ReturnTypeWrapped = true;
-      }
-      break;
+    LineIsFunctionDeclaration =
+        isFunctionDeclarationName(IsCpp, *Tok, Line, ClosingParen);
+    if (LineIsFunctionDeclaration)
+      Tok->setFinalizedType(TT_FunctionDeclarationName);
+    else if (!Tok->isOneOf(TT_CtorDtorDeclName, TT_StartOfName))
+      continue;
+    if (AfterLastAttribute &&
+        mustBreakAfterAttributes(*AfterLastAttribute, Style)) {
+      AfterLastAttribute->MustBreakBefore = true;
+      Line.ReturnTypeWrapped = true;
     }
+    break;
   }
 
-  if (Style.isCpp()) {
+  if (IsCpp) {
     if (!LineIsFunctionDeclaration) {
       // Annotate */&/&& in `operator` function calls as binary operators.
       for (const auto *Tok = Line.First; Tok; Tok = Tok->Next) {
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 80903e7630c8073..dcd05b4175e0b94 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -8479,18 +8479,25 @@ TEST_F(FormatTest, BreaksFunctionDeclarationsWithTrailingTokens) {
       "                   aaaaaaaaaaaaaaaaaaaaaaaaa));");
   verifyFormat("bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "    __attribute__((unused));");
-  verifyGoogleFormat(
+
+  Style = getGoogleStyle();
+  Style.AttributeMacros.push_back("GUARDED_BY");
+  verifyFormat(
       "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
-      "    GUARDED_BY(aaaaaaaaaaaa);");
-  verifyGoogleFormat(
+      "    GUARDED_BY(aaaaaaaaaaaa);",
+      Style);
+  verifyFormat(
       "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
-      "    GUARDED_BY(aaaaaaaaaaaa);");
-  verifyGoogleFormat(
+      "    GUARDED_BY(aaaaaaaaaaaa);",
+      Style);
+  verifyFormat(
       "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa GUARDED_BY(aaaaaaaaaaaa) =\n"
-      "    aaaaaaaa::aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;");
-  verifyGoogleFormat(
+      "    aaaaaaaa::aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;",
+      Style);
+  verifyFormat(
       "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa GUARDED_BY(aaaaaaaaaaaa) =\n"
-      "    aaaaaaaaaaaaaaaaaaaaaaaaa;");
+      "    aaaaaaaaaaaaaaaaaaaaaaaaa;",
+      Style);
 }
 
 TEST_F(FormatTest, FunctionAnnotations) {
@@ -26194,7 +26201,10 @@ TEST_F(FormatTest, RemoveSemicolon) {
 TEST_F(FormatTest, BreakAfterAttributes) {
   FormatStyle Style = getLLVMStyle();
 
-  constexpr StringRef Code("[[nodiscard]] inline int f(int &i);\n"
+  constexpr StringRef Code("[[maybe_unused]] const int i;\n"
+                           "[[foo([[]])]] [[maybe_unused]]\n"
+                           "int j;\n"
+                           "[[nodiscard]] inline int f(int &i);\n"
                            "[[foo([[]])]] [[nodiscard]]\n"
                            "int g(int &i);\n"
                            "[[nodiscard]]\n"
@@ -26211,7 +26221,9 @@ TEST_F(FormatTest, BreakAfterAttributes) {
   verifyNoChange(Code, Style);
 
   Style.BreakAfterAttributes = FormatStyle::ABS_Never;
-  verifyFormat("[[nodiscard]] inline int f(int &i);\n"
+  verifyFormat("[[maybe_unused]] const int i;\n"
+               "[[foo([[]])]] [[maybe_unused]] int j;\n"
+               "[[nodiscard]] inline int f(int &i);\n"
                "[[foo([[]])]] [[nodiscard]] int g(int &i);\n"
                "[[nodiscard]] inline int f(int &i) {\n"
                "  i = 1;\n"
@@ -26224,7 +26236,11 @@ TEST_F(FormatTest, BreakAfterAttributes) {
                Code, Style);
 
   Style.BreakAfterAttributes = FormatStyle::ABS_Always;
-  verifyFormat("[[nodiscard]]\n"
+  verifyFormat("[[maybe_unused]]\n"
+               "const int i;\n"
+               "[[foo([[]])]] [[maybe_unused]]\n"
+               "int j;\n"
+               "[[nodiscard]]\n"
                "inline int f(int &i);\n"
                "[[foo([[]])]] [[nodiscard]]\n"
                "int g(int &i);\n"

[rymiel]

However this doesn't address the second part of the linked issue (with the if) does it?

[owenca]

However this doesn't address the second part of the linked issue (with the if) does it?

Nope! That part is a duplicate of #64474. I was going to add a note in #71563 before merging this PR but now just saw that @FalcoGer closed the old issue and opened the new one, which is essentially the same.

[Meinersbur]

@owenca You can run ninja polly-update-format which will use the in-tree clang-format to reformat all files. git add its changes, git add --amend, then push.

[owenca]

@Meinersbur thanks for the info!