[clang-format] Annotate ctors/dtors as CtorDtorDeclName instead #67955

owenca · 2023-10-02T06:40:17Z

After annotating constructors/destructors as FunctionDeclarationName in commit 0863051, we have seen several issues because ctors/dtors had been treated differently than functions in aligning, wrapping, and indenting.

This patch annotates ctors/dtors as CtorDtorDeclName instead and would effectively revert commit 0468fa0, which is obsolete now.

Fixed #67903.
Fixed #67907.

llvmbot · 2023-10-02T06:41:38Z

@llvm/pr-subscribers-clang-format

Changes

After annotating constructors/destructors as FunctionDeclarationName in commit 0863051, we have seen several issues because ctors/dtors had been treated differently than functions in aligning, wrapping, and indenting.

This patch annotates ctors/dtors as CtorDtorDeclName instead and would effectively revert commit 0468fa0, which is obsolete now.

Fixed #67903.
Fixed #67907.

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

6 Files Affected:

(modified) clang/lib/Format/FormatToken.h (+1)
(modified) clang/lib/Format/TokenAnnotator.cpp (+10-7)
(modified) clang/lib/Format/WhitespaceManager.cpp (+1-5)
(modified) clang/unittests/Format/FormatTest.cpp (+8-2)
(modified) clang/unittests/Format/FormatTestMacroExpansion.cpp (+2-6)
(modified) clang/unittests/Format/TokenAnnotatorTest.cpp (+33-21)

diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index dbd3a6e70f037ef..5877b0a6124742a 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -61,6 +61,7 @@ namespace format {
   TYPE(CSharpStringLiteral)                                                    \
   TYPE(CtorInitializerColon)                                                   \
   TYPE(CtorInitializerComma)                                                   \
+  TYPE(CtorDtorDeclName)                                                       \
   TYPE(DesignatedInitializerLSquare)                                           \
   TYPE(DesignatedInitializerPeriod)                                            \
   TYPE(DictLiteral)                                                            \
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index ae2cbbdce934618..dfb059ac8464ed2 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -3211,9 +3211,6 @@ static bool isCtorOrDtorName(const FormatToken *Tok) {
 }
 
 void TokenAnnotator::annotate(AnnotatedLine &Line) {
-  for (auto &Child : Line.Children)
-    annotate(*Child);
-
   AnnotatingParser Parser(Style, Line, Keywords, Scopes);
   Line.Type = Parser.parseLine();
 
@@ -3234,7 +3231,7 @@ void TokenAnnotator::annotate(AnnotatedLine &Line) {
     auto *Tok = getFunctionName(Line);
     if (Tok && ((!Scopes.empty() && Scopes.back() == ST_Class) ||
                 Line.endsWith(TT_FunctionLBrace) || isCtorOrDtorName(Tok))) {
-      Tok->setFinalizedType(TT_FunctionDeclarationName);
+      Tok->setFinalizedType(TT_CtorDtorDeclName);
     }
   }
 
@@ -3247,6 +3244,9 @@ void TokenAnnotator::annotate(AnnotatedLine &Line) {
 
   Line.First->SpacesRequiredBefore = 1;
   Line.First->CanBreakBefore = Line.First->MustBreakBefore;
+
+  for (auto &Child : Line.Children)
+    annotate(*Child);
 }
 
 // This function heuristically determines whether 'Current' starts the name of a
@@ -3446,9 +3446,12 @@ void TokenAnnotator::calculateFormattingInformation(AnnotatedLine &Line) const {
        Tok = Tok->Next) {
     if (Tok->Previous->EndsCppAttributeGroup)
       AfterLastAttribute = Tok;
-    if (isFunctionDeclarationName(Style.isCpp(), *Tok, Line)) {
-      LineIsFunctionDeclaration = true;
-      Tok->setFinalizedType(TT_FunctionDeclarationName);
+    if (const bool IsCtorOrDtor = Tok->is(TT_CtorDtorDeclName);
+        IsCtorOrDtor || isFunctionDeclarationName(Style.isCpp(), *Tok, Line)) {
+      if (!IsCtorOrDtor) {
+        LineIsFunctionDeclaration = true;
+        Tok->setFinalizedType(TT_FunctionDeclarationName);
+      }
       if (AfterLastAttribute &&
           mustBreakAfterAttributes(*AfterLastAttribute, Style)) {
         AfterLastAttribute->MustBreakBefore = true;
diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp
index 762729d1c4166a5..1790a9df42b5d14 100644
--- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -974,11 +974,7 @@ void WhitespaceManager::alignConsecutiveDeclarations() {
   AlignTokens(
       Style,
       [](Change const &C) {
-        if (C.Tok->is(TT_FunctionDeclarationName) && C.Tok->Previous &&
-            C.Tok->Previous->isNot(tok::tilde)) {
-          return true;
-        }
-        if (C.Tok->is(TT_FunctionTypeLParen))
+        if (C.Tok->isOneOf(TT_FunctionDeclarationName, TT_FunctionTypeLParen))
           return true;
         if (C.Tok->isNot(TT_StartOfName))
           return false;
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 0403de8a9a65594..0ab57398a6cc319 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -10622,6 +10622,12 @@ TEST_F(FormatTest, WrapsAtNestedNameSpecifiers) {
   verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa::\n"
                "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "        .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa();");
+
+  verifyFormat(
+      "LongClassNameToShowTheIssue::AndAnotherLongClassNameToShowTheIssue::\n"
+      "    AndAnotherLongClassNameToShowTheIssue() {}\n"
+      "LongClassNameToShowTheIssue::AndAnotherLongClassNameToShowTheIssue::\n"
+      "    ~AndAnotherLongClassNameToShowTheIssue() {}");
 }
 
 TEST_F(FormatTest, UnderstandsTemplateParameters) {
@@ -16339,7 +16345,7 @@ TEST_F(FormatTest, ConfigurableSpaceBeforeParens) {
 
   verifyFormat("int f();", SpaceFuncDef);
   verifyFormat("void f (int a, T b) {}", SpaceFuncDef);
-  verifyFormat("A::A () : a(1) {}", SpaceFuncDef);
+  verifyFormat("A::A() : a(1) {}", SpaceFuncDef);
   verifyFormat("void f() __attribute__((asdf));", SpaceFuncDef);
   verifyFormat("#define A(x) x", SpaceFuncDef);
   verifyFormat("#define A (x) x", SpaceFuncDef);
@@ -16364,7 +16370,7 @@ TEST_F(FormatTest, ConfigurableSpaceBeforeParens) {
   // verifyFormat("T A::operator() () {}", SpaceFuncDef);
   verifyFormat("auto lambda = [] () { return 0; };", SpaceFuncDef);
   verifyFormat("int x = int(y);", SpaceFuncDef);
-  verifyFormat("M (std::size_t R, std::size_t C) : C(C), data(R) {}",
+  verifyFormat("M(std::size_t R, std::size_t C) : C(C), data(R) {}",
                SpaceFuncDef);
 
   FormatStyle SpaceIfMacros = getLLVMStyle();
diff --git a/clang/unittests/Format/FormatTestMacroExpansion.cpp b/clang/unittests/Format/FormatTestMacroExpansion.cpp
index 1ac5ac0d84f1251..741271a96dad739 100644
--- a/clang/unittests/Format/FormatTestMacroExpansion.cpp
+++ b/clang/unittests/Format/FormatTestMacroExpansion.cpp
@@ -47,9 +47,7 @@ TEST_F(FormatTestMacroExpansion, UnexpandConfiguredMacros) {
     { ID(a *b); });
 )",
                Style);
-  verifyIncompleteFormat(R"(ID3({, ID(a *b),
-  ;
-  });
+  verifyIncompleteFormat(R"(ID3({, ID(a *b), ; });
 )",
                          Style);
 
@@ -251,9 +249,7 @@ TEST_F(FormatTestMacroExpansion,
        ContinueFormattingAfterUnclosedParensAfterObjectLikeMacro) {
   FormatStyle Style = getLLVMStyle();
   Style.Macros.push_back("O=class {");
-  verifyIncompleteFormat("O(auto x = [](){\n"
-                         "    f();}",
-                         Style);
+  verifyIncompleteFormat("O(auto x = [](){f();}", Style);
 }
 
 } // namespace
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp b/clang/unittests/Format/TokenAnnotatorTest.cpp
index cf66076c86216aa..738e632246fee55 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1631,65 +1631,77 @@ TEST_F(TokenAnnotatorTest, UnderstandsFunctionDeclarationNames) {
   ASSERT_EQ(Tokens.size(), 12u) << Tokens;
   EXPECT_TOKEN(Tokens[1], tok::identifier, TT_FunctionDeclarationName);
 
-  Tokens = annotate("class Foo { public: Foo(); };");
+  Tokens = annotate("#define FOO Foo::\n"
+                    "FOO Foo();");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[6], tok::identifier, TT_FunctionDeclarationName);
+
+  Tokens = annotate("struct Foo {\n"
+                    "  Bar (*func)();\n"
+                    "};");
+  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::identifier, TT_Unknown);
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_FunctionTypeLParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsCtorAndDtorDeclNames) {
+  auto Tokens = annotate("class Foo { public: Foo(); };");
   ASSERT_EQ(Tokens.size(), 12u) << Tokens;
-  EXPECT_TOKEN(Tokens[5], tok::identifier, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[5], tok::identifier, TT_CtorDtorDeclName);
 
   Tokens = annotate("class Foo { public: ~Foo(); };");
   ASSERT_EQ(Tokens.size(), 13u) << Tokens;
-  EXPECT_TOKEN(Tokens[6], tok::identifier, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[6], tok::identifier, TT_CtorDtorDeclName);
 
   Tokens = annotate("struct Foo { [[deprecated]] Foo() {} };");
   ASSERT_EQ(Tokens.size(), 16u) << Tokens;
-  EXPECT_TOKEN(Tokens[8], tok::identifier, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[8], tok::identifier, TT_CtorDtorDeclName);
   EXPECT_TOKEN(Tokens[11], tok::l_brace, TT_FunctionLBrace);
 
   Tokens = annotate("struct Foo { [[deprecated]] ~Foo() {} };");
   ASSERT_EQ(Tokens.size(), 17u) << Tokens;
-  EXPECT_TOKEN(Tokens[9], tok::identifier, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[9], tok::identifier, TT_CtorDtorDeclName);
   EXPECT_TOKEN(Tokens[12], tok::l_brace, TT_FunctionLBrace);
 
   Tokens = annotate("struct Foo { Foo() [[deprecated]] {} };");
   ASSERT_EQ(Tokens.size(), 16u) << Tokens;
-  EXPECT_TOKEN(Tokens[3], tok::identifier, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[3], tok::identifier, TT_CtorDtorDeclName);
   EXPECT_TOKEN(Tokens[11], tok::l_brace, TT_FunctionLBrace);
 
   Tokens = annotate("struct Foo { ~Foo() [[deprecated]] {} };");
   ASSERT_EQ(Tokens.size(), 17u) << Tokens;
-  EXPECT_TOKEN(Tokens[4], tok::identifier, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[4], tok::identifier, TT_CtorDtorDeclName);
   EXPECT_TOKEN(Tokens[12], tok::l_brace, TT_FunctionLBrace);
 
   Tokens = annotate("struct Foo { [[deprecated]] explicit Foo() {} };");
   ASSERT_EQ(Tokens.size(), 17u) << Tokens;
-  EXPECT_TOKEN(Tokens[9], tok::identifier, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[9], tok::identifier, TT_CtorDtorDeclName);
   EXPECT_TOKEN(Tokens[12], tok::l_brace, TT_FunctionLBrace);
 
   Tokens = annotate("struct Foo { virtual [[deprecated]] ~Foo() {} };");
   ASSERT_EQ(Tokens.size(), 18u) << Tokens;
-  EXPECT_TOKEN(Tokens[10], tok::identifier, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[10], tok::identifier, TT_CtorDtorDeclName);
   EXPECT_TOKEN(Tokens[13], tok::l_brace, TT_FunctionLBrace);
 
   Tokens = annotate("Foo::Foo() {}");
   ASSERT_EQ(Tokens.size(), 8u) << Tokens;
-  EXPECT_TOKEN(Tokens[2], tok::identifier, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[2], tok::identifier, TT_CtorDtorDeclName);
   EXPECT_TOKEN(Tokens[5], tok::l_brace, TT_FunctionLBrace);
 
   Tokens = annotate("Foo::~Foo() {}");
   ASSERT_EQ(Tokens.size(), 9u) << Tokens;
-  EXPECT_TOKEN(Tokens[3], tok::identifier, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[3], tok::identifier, TT_CtorDtorDeclName);
   EXPECT_TOKEN(Tokens[6], tok::l_brace, TT_FunctionLBrace);
 
-  Tokens = annotate("#define FOO Foo::\n"
-                    "FOO Foo();");
-  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
-  EXPECT_TOKEN(Tokens[6], tok::identifier, TT_FunctionDeclarationName);
-
-  Tokens = annotate("struct Foo {\n"
-                    "  Bar (*func)();\n"
+  Tokens = annotate("struct Test {\n"
+                    "  Test()\n"
+                    "      : l([] {\n"
+                    "          Short::foo();\n"
+                    "        }) {}\n"
                     "};");
-  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
-  EXPECT_TOKEN(Tokens[3], tok::identifier, TT_Unknown);
-  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_FunctionTypeLParen);
+  ASSERT_EQ(Tokens.size(), 25u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::identifier, TT_CtorDtorDeclName);
+  EXPECT_TOKEN(Tokens[14], tok::identifier, TT_Unknown);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsC11GenericSelection) {

After annotating constructors/destructors as FunctionDeclarationName in commit 0863051, we have seen several issues because ctors/dtors had been treated differently than functions in aligning, wrapping, and indenting. This patch annotates ctors/dtors as CtorDtorDeclName instead and would effectively revert commit 0468fa0, which is obsolete now. Fixed llvm#67903. Fixed llvm#67907.

mydeveloperday

Looks like it fixes some bugs in the tests too!

clang/lib/Format/TokenAnnotator.cpp

clang/unittests/Format/FormatTest.cpp

clang/lib/Format/TokenAnnotator.cpp

rymiel · 2023-10-03T19:45:30Z

https://buildkite.com/llvm-project/clang-ci/builds/4275#018af20e-3d3c-4344-b92d-88ac8b09b484 Tests do not pass?

owenca · 2023-10-03T20:41:38Z

https://buildkite.com/llvm-project/clang-ci/builds/4275#018af20e-3d3c-4344-b92d-88ac8b09b484 Tests do not pass?

I saw that but don't know why. Did you have the same failures? I'll push another commit shortly to trigger buildkite again.

dyung · 2023-10-04T01:15:53Z

I'm seeing the same failures on our linux/Windows build bots:
https://lab.llvm.org/buildbot/#/builders/139/builds/50966
https://lab.llvm.org/buildbot/#/builders/216/builds/28315

…ad (#67955)" This reverts commit 6a621ed as it caused buildbots to fail.

owenca · 2023-10-04T01:26:07Z

Reverted in d08fcc8.

…ad (#67955)" Reland 6a621ed which failed on Windows but not macOS. The failures were caused by moving the annotation of the children from the top to the bottom of TokenAnnotator::annotate(), resulting in invalid lines including incomplete ones being skipped.

Local branch amd-gfx 696586e Merged main:548d67a0393c into amd-gfx:b60b0143910c Remote branch main 7e856d1 Reland "[clang-format] Annotate ctors/dtors as CtorDtorDeclName instead (llvm#67955)"

llvmbot added the clang-format label Oct 2, 2023

owenca force-pushed the CtorDtor branch from 62b4e7b to 15d3707 Compare October 2, 2023 20:21

owenca requested review from HazardyKnusperkeks, mydeveloperday and rymiel October 2, 2023 20:24

mydeveloperday approved these changes Oct 2, 2023

View reviewed changes

HazardyKnusperkeks reviewed Oct 3, 2023

View reviewed changes

clang/lib/Format/TokenAnnotator.cpp Show resolved Hide resolved

HazardyKnusperkeks reviewed Oct 3, 2023

View reviewed changes

clang/unittests/Format/FormatTest.cpp Show resolved Hide resolved

clang/lib/Format/TokenAnnotator.cpp Show resolved Hide resolved

[clang-format] Simplify a test case

0528141

HazardyKnusperkeks approved these changes Oct 3, 2023

View reviewed changes

owenca merged commit 6a621ed into llvm:main Oct 4, 2023

owenca deleted the CtorDtor branch October 4, 2023 01:02

owenca added a commit that referenced this pull request Oct 4, 2023

Revert "[clang-format] Annotate ctors/dtors as CtorDtorDeclName inste…

d08fcc8

…ad (#67955)" This reverts commit 6a621ed as it caused buildbots to fail.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang-format] Annotate ctors/dtors as CtorDtorDeclName instead #67955

[clang-format] Annotate ctors/dtors as CtorDtorDeclName instead #67955

owenca commented Oct 2, 2023

llvmbot commented Oct 2, 2023

mydeveloperday left a comment

rymiel commented Oct 3, 2023

owenca commented Oct 3, 2023

dyung commented Oct 4, 2023

owenca commented Oct 4, 2023

[clang-format] Annotate ctors/dtors as CtorDtorDeclName instead #67955

[clang-format] Annotate ctors/dtors as CtorDtorDeclName instead #67955

Conversation

owenca commented Oct 2, 2023

llvmbot commented Oct 2, 2023

mydeveloperday left a comment

Choose a reason for hiding this comment

rymiel commented Oct 3, 2023

owenca commented Oct 3, 2023

dyung commented Oct 4, 2023

owenca commented Oct 4, 2023