[clang] [ClangFormat] Fix formatting bugs. (PR #76245)

via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 10 02:32:48 PST 2024


https://github.com/r4nt updated https://github.com/llvm/llvm-project/pull/76245

>From 52cb11f0279dbd9f65f15e81f44869cfac00d544 Mon Sep 17 00:00:00 2001
From: Manuel Klimek <klimek at google.com>
Date: Thu, 2 Mar 2023 14:00:35 +0000
Subject: [PATCH 1/3] [ClangFormat] Fix formatting bugs.

1. There are multiple calls to addFakeParenthesis; move the guard to not
   assign fake parenthesis into the function to make sure we cover all
   calls.
2. MustBreakBefore can be set on a token in two cases: either during
   unwrapped line parsing, or later, during token annotation. We must
   keep the latter, but reset the former.
3. Added a test to document that the intended behavior of preferring not to
   break between a return type and a function identifier.
   For example, with MOCK_METHOD(r, n, a)=r n a, the code
   MOCK_METHOD(void, f, (int a, int b)) should prefer the same breaks as
   the expanded void f(int a, int b).
---
 clang/lib/Format/FormatToken.h                | 26 +++++++++----
 clang/lib/Format/TokenAnnotator.cpp           | 13 +++----
 clang/lib/Format/UnwrappedLineFormatter.cpp   | 10 +++--
 clang/lib/Format/UnwrappedLineParser.cpp      |  2 +
 .../Format/FormatTestMacroExpansion.cpp       | 38 +++++++++++++++++++
 5 files changed, 70 insertions(+), 19 deletions(-)

diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index 3f9664f8f78a3e..b1e3ae8ab303d6 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -275,14 +275,15 @@ class AnnotatedLine;
 struct FormatToken {
   FormatToken()
       : HasUnescapedNewline(false), IsMultiline(false), IsFirst(false),
-        MustBreakBefore(false), IsUnterminatedLiteral(false),
-        CanBreakBefore(false), ClosesTemplateDeclaration(false),
-        StartsBinaryExpression(false), EndsBinaryExpression(false),
-        PartOfMultiVariableDeclStmt(false), ContinuesLineCommentSection(false),
-        Finalized(false), ClosesRequiresClause(false),
-        EndsCppAttributeGroup(false), BlockKind(BK_Unknown),
-        Decision(FD_Unformatted), PackingKind(PPK_Inconclusive),
-        TypeIsFinalized(false), Type(TT_Unknown) {}
+        MustBreakBefore(false), MustBreakBeforeFinalized(false),
+        IsUnterminatedLiteral(false), CanBreakBefore(false),
+        ClosesTemplateDeclaration(false), StartsBinaryExpression(false),
+        EndsBinaryExpression(false), PartOfMultiVariableDeclStmt(false),
+        ContinuesLineCommentSection(false), Finalized(false),
+        ClosesRequiresClause(false), EndsCppAttributeGroup(false),
+        BlockKind(BK_Unknown), Decision(FD_Unformatted),
+        PackingKind(PPK_Inconclusive), TypeIsFinalized(false),
+        Type(TT_Unknown) {}
 
   /// The \c Token.
   Token Tok;
@@ -318,6 +319,10 @@ struct FormatToken {
   /// before the token.
   unsigned MustBreakBefore : 1;
 
+  /// Whether MustBreakBefore is finalized during parsing and must not
+  /// be reset between runs.
+  unsigned MustBreakBeforeFinalized : 1;
+
   /// Set to \c true if this token is an unterminated literal.
   unsigned IsUnterminatedLiteral : 1;
 
@@ -416,10 +421,15 @@ struct FormatToken {
   /// to another one please use overwriteFixedType, or even better remove the
   /// need to reassign the type.
   void setFinalizedType(TokenType T) {
+    if (MacroCtx && MacroCtx->Role == MR_UnexpandedArg)
+      return;
+
     Type = T;
     TypeIsFinalized = true;
   }
   void overwriteFixedType(TokenType T) {
+    if (MacroCtx && MacroCtx->Role == MR_UnexpandedArg)
+      return;
     TypeIsFinalized = false;
     setType(T);
   }
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index f3551af3424396..c26b248a3b2d40 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2769,13 +2769,6 @@ class ExpressionParser {
       // Consume operators with higher precedence.
       parse(Precedence + 1);
 
-      // Do not assign fake parenthesis to tokens that are part of an
-      // unexpanded macro call. The line within the macro call contains
-      // the parenthesis and commas, and we will not find operators within
-      // that structure.
-      if (Current && Current->MacroParent)
-        break;
-
       int CurrentPrecedence = getCurrentPrecedence();
 
       if (Precedence == CurrentPrecedence && Current &&
@@ -2919,6 +2912,12 @@ class ExpressionParser {
 
   void addFakeParenthesis(FormatToken *Start, prec::Level Precedence,
                           FormatToken *End = nullptr) {
+    // Do not assign fake parenthesis to tokens that are part of an
+    // unexpanded macro call. The line within the macro call contains
+    // the parenthesis and commas, and we will not find operators within
+    // that structure.
+    if (Start->MacroParent) return;
+
     Start->FakeLParens.push_back(Precedence);
     if (Precedence > prec::Unknown)
       Start->StartsBinaryExpression = true;
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 56077499c39d53..27983a330ac40a 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -954,13 +954,15 @@ static void markFinalized(FormatToken *Tok) {
       // will be modified as unexpanded arguments (as part of the macro call
       // formatting) in the next pass.
       Tok->MacroCtx->Role = MR_UnexpandedArg;
-      // Reset whether spaces are required before this token, as that is context
-      // dependent, and that context may change when formatting the macro call.
-      // For example, given M(x) -> 2 * x, and the macro call M(var),
-      // the token 'var' will have SpacesRequiredBefore = 1 after being
+      // Reset whether spaces or a line break are required before this token, as
+      // that is context dependent, and that context may change when formatting
+      // the macro call.  For example, given M(x) -> 2 * x, and the macro call
+      // M(var), the token 'var' will have SpacesRequiredBefore = 1 after being
       // formatted as part of the expanded macro, but SpacesRequiredBefore = 0
       // for its position within the macro call.
       Tok->SpacesRequiredBefore = 0;
+      if (!Tok->MustBreakBeforeFinalized)
+        Tok->MustBreakBefore = 0;
     } else {
       Tok->Finalized = true;
     }
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 684609747a5513..942aaff3c456df 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -4675,6 +4675,7 @@ void UnwrappedLineParser::readToken(int LevelDifference) {
         conditionalCompilationEnd();
       FormatTok = Tokens->getNextToken();
       FormatTok->MustBreakBefore = true;
+      FormatTok->MustBreakBeforeFinalized = true;
     }
 
     auto IsFirstNonCommentOnLine = [](bool FirstNonCommentOnLine,
@@ -4891,6 +4892,7 @@ void UnwrappedLineParser::pushToken(FormatToken *Tok) {
   Line->Tokens.push_back(UnwrappedLineNode(Tok));
   if (MustBreakBeforeNextToken) {
     Line->Tokens.back().Tok->MustBreakBefore = true;
+    Line->Tokens.back().Tok->MustBreakBeforeFinalized = true;
     MustBreakBeforeNextToken = false;
   }
 }
diff --git a/clang/unittests/Format/FormatTestMacroExpansion.cpp b/clang/unittests/Format/FormatTestMacroExpansion.cpp
index 68250234f4201e..afc88f0eb75673 100644
--- a/clang/unittests/Format/FormatTestMacroExpansion.cpp
+++ b/clang/unittests/Format/FormatTestMacroExpansion.cpp
@@ -255,6 +255,44 @@ TEST_F(FormatTestMacroExpansion,
                          Style);
 }
 
+TEST_F(FormatTestMacroExpansion, CommaAsOperator) {
+  FormatStyle Style = getGoogleStyle();
+  Style.Macros.push_back("ASSIGN_OR_RETURN(a, b, c)=a=(b); if(x) c");
+  verifyFormat(R"(ASSIGN_OR_RETURN(auto reader,
+                 logs::proxy::Reader::NewReader(log_filename, access_options,
+                                                reader_options),
+                 _ << aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa << aaaaaa << aaaaaaaaa
+                   << aaaaaaaaaaaa << aaaaaaaaaaa << aaaaaaaaaaaa);
+)", Style);
+}
+
+TEST_F(FormatTestMacroExpansion, ForcedBreakDiffers) {
+  FormatStyle Style = getGoogleStyle();
+  Style.Macros.push_back("ASSIGN_OR_RETURN(a, b)=a=(b)");
+  EXPECT_EQ(R"(
+//
+ASSIGN_OR_RETURN(const lllllllllllllllllllllllllll xxxxxxxxxxxxxxx,
+                 aaaaaaaaaaaaaaaaaaaaaa(
+                     bbbbbbbbb, ccccccccccccccccccccccccccccc, dddddddddddd));
+)", format(R"(
+//
+ASSIGN_OR_RETURN(const lllllllllllllllllllllllllll xxxxxxxxxxxxxxx,
+                 aaaaaaaaaaaaaaaaaaaaaa(
+                     bbbbbbbbb, ccccccccccccccccccccccccccccc, dddddddddddd));
+)", Style));
+}
+
+TEST_F(FormatTestMacroExpansion, PreferNotBreakingBetweenReturnTypeAndFunction) {
+  FormatStyle Style = getGoogleStyle();
+  Style.Macros.push_back("MOCK_METHOD(r, n, a, s)=r n a s");
+  // In the expanded code, we parse a full function signature, and afterwards
+  // know that we prefer not to break before the function name.
+  verifyFormat(R"(MOCK_METHOD(
+    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
+    (cccccccccccccccccccccccccccccc), (const, override));
+)", Style);
+}
+
 } // namespace
 } // namespace test
 } // namespace format

>From dec1fb75495461efc9ea370a058ce17d9ca352d4 Mon Sep 17 00:00:00 2001
From: Manuel Klimek <klimek at google.com>
Date: Tue, 9 Jan 2024 13:28:34 +0000
Subject: [PATCH 2/3] fixup! [ClangFormat] Fix formatting bugs.

---
 clang/lib/Format/FormatToken.h                |  1 -
 clang/lib/Format/TokenAnnotator.cpp           |  3 +-
 .../Format/FormatTestMacroExpansion.cpp       | 47 +++++++++----------
 3 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index b1e3ae8ab303d6..981592aa094a7a 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -423,7 +423,6 @@ struct FormatToken {
   void setFinalizedType(TokenType T) {
     if (MacroCtx && MacroCtx->Role == MR_UnexpandedArg)
       return;
-
     Type = T;
     TypeIsFinalized = true;
   }
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index c26b248a3b2d40..6b153759ad6982 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2916,7 +2916,8 @@ class ExpressionParser {
     // unexpanded macro call. The line within the macro call contains
     // the parenthesis and commas, and we will not find operators within
     // that structure.
-    if (Start->MacroParent) return;
+    if (Start->MacroParent)
+      return;
 
     Start->FakeLParens.push_back(Precedence);
     if (Precedence > prec::Unknown)
diff --git a/clang/unittests/Format/FormatTestMacroExpansion.cpp b/clang/unittests/Format/FormatTestMacroExpansion.cpp
index afc88f0eb75673..f17e0adcfc21eb 100644
--- a/clang/unittests/Format/FormatTestMacroExpansion.cpp
+++ b/clang/unittests/Format/FormatTestMacroExpansion.cpp
@@ -257,40 +257,37 @@ TEST_F(FormatTestMacroExpansion,
 
 TEST_F(FormatTestMacroExpansion, CommaAsOperator) {
   FormatStyle Style = getGoogleStyle();
-  Style.Macros.push_back("ASSIGN_OR_RETURN(a, b, c)=a=(b); if(x) c");
-  verifyFormat(R"(ASSIGN_OR_RETURN(auto reader,
-                 logs::proxy::Reader::NewReader(log_filename, access_options,
-                                                reader_options),
-                 _ << aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa << aaaaaa << aaaaaaaaa
-                   << aaaaaaaaaaaa << aaaaaaaaaaa << aaaaaaaaaaaa);
-)", Style);
+  Style.ColumnLimit = 42;
+  Style.Macros.push_back("MACRO(a, b, c)=a=(b); if(x) c");
+  verifyFormat("MACRO(auto a,\n"
+               "      looooongfunction(first, second,\n"
+               "                       third),\n"
+               "      fourth);",
+               Style);
 }
 
 TEST_F(FormatTestMacroExpansion, ForcedBreakDiffers) {
   FormatStyle Style = getGoogleStyle();
-  Style.Macros.push_back("ASSIGN_OR_RETURN(a, b)=a=(b)");
-  EXPECT_EQ(R"(
-//
-ASSIGN_OR_RETURN(const lllllllllllllllllllllllllll xxxxxxxxxxxxxxx,
-                 aaaaaaaaaaaaaaaaaaaaaa(
-                     bbbbbbbbb, ccccccccccccccccccccccccccccc, dddddddddddd));
-)", format(R"(
-//
-ASSIGN_OR_RETURN(const lllllllllllllllllllllllllll xxxxxxxxxxxxxxx,
-                 aaaaaaaaaaaaaaaaaaaaaa(
-                     bbbbbbbbb, ccccccccccccccccccccccccccccc, dddddddddddd));
-)", Style));
+  Style.ColumnLimit = 40;
+  Style.Macros.push_back("MACRO(a, b)=a=(b)");
+  verifyFormat("//\n"
+               "MACRO(const type variable,\n"
+               "      functtioncall(\n"
+               "          first, longsecondarg, third));\n",
+               Style);
 }
 
-TEST_F(FormatTestMacroExpansion, PreferNotBreakingBetweenReturnTypeAndFunction) {
+TEST_F(FormatTestMacroExpansion,
+       PreferNotBreakingBetweenReturnTypeAndFunction) {
   FormatStyle Style = getGoogleStyle();
-  Style.Macros.push_back("MOCK_METHOD(r, n, a, s)=r n a s");
+  Style.ColumnLimit = 22;
+  Style.Macros.push_back("MOCK_METHOD(r, n, a)=r n a");
   // In the expanded code, we parse a full function signature, and afterwards
   // know that we prefer not to break before the function name.
-  verifyFormat(R"(MOCK_METHOD(
-    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
-    (cccccccccccccccccccccccccccccc), (const, override));
-)", Style);
+  verifyFormat("MOCK_METHOD(\n"
+               "    type, variable,\n"
+               "    (type));\n",
+               Style);
 }
 
 } // namespace

>From 20461a15a1595121832d3548b08d91d35d2452e9 Mon Sep 17 00:00:00 2001
From: Manuel Klimek <klimek at google.com>
Date: Wed, 10 Jan 2024 10:32:27 +0000
Subject: [PATCH 3/3] fixup! fixup! [ClangFormat] Fix formatting bugs.

---
 clang/unittests/Format/FormatTestMacroExpansion.cpp | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/clang/unittests/Format/FormatTestMacroExpansion.cpp b/clang/unittests/Format/FormatTestMacroExpansion.cpp
index f17e0adcfc21eb..653ec2a94c64da 100644
--- a/clang/unittests/Format/FormatTestMacroExpansion.cpp
+++ b/clang/unittests/Format/FormatTestMacroExpansion.cpp
@@ -256,8 +256,7 @@ TEST_F(FormatTestMacroExpansion,
 }
 
 TEST_F(FormatTestMacroExpansion, CommaAsOperator) {
-  FormatStyle Style = getGoogleStyle();
-  Style.ColumnLimit = 42;
+  FormatStyle Style = getGoogleStyleWithColumns(42);
   Style.Macros.push_back("MACRO(a, b, c)=a=(b); if(x) c");
   verifyFormat("MACRO(auto a,\n"
                "      looooongfunction(first, second,\n"
@@ -267,26 +266,24 @@ TEST_F(FormatTestMacroExpansion, CommaAsOperator) {
 }
 
 TEST_F(FormatTestMacroExpansion, ForcedBreakDiffers) {
-  FormatStyle Style = getGoogleStyle();
-  Style.ColumnLimit = 40;
+  FormatStyle Style = getGoogleStyleWithColumns(40);
   Style.Macros.push_back("MACRO(a, b)=a=(b)");
   verifyFormat("//\n"
                "MACRO(const type variable,\n"
                "      functtioncall(\n"
-               "          first, longsecondarg, third));\n",
+               "          first, longsecondarg, third));",
                Style);
 }
 
 TEST_F(FormatTestMacroExpansion,
        PreferNotBreakingBetweenReturnTypeAndFunction) {
-  FormatStyle Style = getGoogleStyle();
-  Style.ColumnLimit = 22;
+  FormatStyle Style = getGoogleStyleWithColumns(22);
   Style.Macros.push_back("MOCK_METHOD(r, n, a)=r n a");
   // In the expanded code, we parse a full function signature, and afterwards
   // know that we prefer not to break before the function name.
   verifyFormat("MOCK_METHOD(\n"
                "    type, variable,\n"
-               "    (type));\n",
+               "    (type));",
                Style);
 }
 



More information about the cfe-commits mailing list