[clang] [clang-format] Skip PP directives when determining brace kind (PR #69473)

[Emilia Kond via cfe-commits]

https://github.com/rymiel updated https://github.com/llvm/llvm-project/pull/69473

>From 1e4e878d251b44aa68fb8bc6422a9ec4e3de4e12 Mon Sep 17 00:00:00 2001
From: Emilia Kond <emilia at rymiel.space>
Date: Wed, 18 Oct 2023 18:03:32 +0300
Subject: [PATCH 1/2] [clang-format] Skip PP directives when determining brace
 kind

Pull request #65409 changed the brace kind heuristic to not treat a
preprocessor if directive as a in statement, however, this caused some
regressions.

If the contents of a brace don't immediately determine the brace kind,
the heuristic will look at the characters immediately before and after
the closing brace to determine the brace type.

Unfortunately, if the closing brace was preceded by a preprocessor
directive, for example `#endif`, the preceding token was `endif`, seen
as just an identifier, so the braces were understood as a braced list.

This patch fixes this behaviour by skipping all preprocessor directives
when calculating brace types. Comments were already being skipped, so
now preprocessor lines are skipped alongside comments.

Fixes https://github.com/llvm/llvm-project/issues/68404
---
 clang/lib/Format/UnwrappedLineParser.cpp      | 23 +++++++++++--------
 clang/unittests/Format/TokenAnnotatorTest.cpp | 15 ++++++++++++
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 708b70489a114e3..90e68e875fd462b 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -491,11 +491,19 @@ void UnwrappedLineParser::calculateBraceTypes(bool ExpectClassBody) {
   SmallVector<StackEntry, 8> LBraceStack;
   assert(Tok->is(tok::l_brace));
   do {
-    // Get next non-comment token.
-    FormatToken *NextTok;
-    do {
-      NextTok = Tokens->getNextToken();
-    } while (NextTok->is(tok::comment));
+    // Get next non-comment, non-preprocessor token.
+    FormatToken *NextTok = Tokens->getNextToken();
+    while (NextTok->is(tok::comment) ||
+           (NextTok->is(tok::hash) && isOnNewLine(*NextTok))) {
+      while (NextTok->is(tok::comment))
+        NextTok = Tokens->getNextToken();
+      while (NextTok->is(tok::hash) && isOnNewLine(*NextTok)) {
+        ScopedMacroState MacroState(*Line, Tokens, NextTok);
+        do {
+          NextTok = Tokens->getNextToken();
+        } while (NextTok->isNot(tok::eof));
+      }
+    }
 
     switch (Tok->Tok.getKind()) {
     case tok::l_brace:
@@ -611,12 +619,9 @@ void UnwrappedLineParser::calculateBraceTypes(bool ExpectClassBody) {
       if (Tok->isNot(TT_StatementMacro))
         break;
       [[fallthrough]];
-    case tok::kw_if:
-      if (PrevTok->is(tok::hash))
-        break;
-      [[fallthrough]];
     case tok::at:
     case tok::semi:
+    case tok::kw_if:
     case tok::kw_while:
     case tok::kw_for:
     case tok::kw_switch:
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp b/clang/unittests/Format/TokenAnnotatorTest.cpp
index 2d046947996693f..8c76823fdeb6ecd 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -2134,6 +2134,7 @@ TEST_F(TokenAnnotatorTest, UnderstandConstructors) {
   EXPECT_TOKEN(Tokens[11], tok::l_paren, TT_Unknown);
   EXPECT_TOKEN(Tokens[12], tok::r_paren, TT_Unknown);
   EXPECT_TOKEN(Tokens[13], tok::l_brace, TT_FunctionLBrace);
+  EXPECT_BRACE_KIND(Tokens[13], BK_Block);
 
   Tokens = annotate("Class::Class() : BaseClass{}, Member{} {}");
   ASSERT_EQ(Tokens.size(), 16u) << Tokens;
@@ -2146,6 +2147,20 @@ TEST_F(TokenAnnotatorTest, UnderstandConstructors) {
   EXPECT_TOKEN(Tokens[11], tok::l_brace, TT_Unknown);
   EXPECT_TOKEN(Tokens[12], tok::r_brace, TT_Unknown);
   EXPECT_TOKEN(Tokens[13], tok::l_brace, TT_FunctionLBrace);
+  EXPECT_BRACE_KIND(Tokens[13], BK_Block);
+
+  Tokens = annotate("class Class {\n"
+                    "  Class() : BaseClass() {\n"
+                    "#if 0\n"
+                    "    // comment\n"
+                    "#endif\n"
+                    "  }\n"
+                    "  Class f();\n"
+                    "}");
+  ASSERT_EQ(Tokens.size(), 25u) << Tokens;
+  EXPECT_TOKEN(Tokens[6], tok::colon, TT_CtorInitializerColon);
+  EXPECT_TOKEN(Tokens[10], tok::l_brace, TT_FunctionLBrace);
+  EXPECT_BRACE_KIND(Tokens[10], BK_Block);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsConditionParens) {

>From d66e797768d3a97a15973ad780e1ab76364f581b Mon Sep 17 00:00:00 2001
From: Emilia Kond <emilia at rymiel.space>
Date: Fri, 20 Oct 2023 21:26:58 +0300
Subject: [PATCH 2/2] Comments from review

Co-authored-by: Owen Pan <owenpiano at gmail.com>
---
 clang/lib/Format/UnwrappedLineParser.cpp | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 90e68e875fd462b..6d5d87d6606a277 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -492,17 +492,16 @@ void UnwrappedLineParser::calculateBraceTypes(bool ExpectClassBody) {
   assert(Tok->is(tok::l_brace));
   do {
     // Get next non-comment, non-preprocessor token.
-    FormatToken *NextTok = Tokens->getNextToken();
-    while (NextTok->is(tok::comment) ||
-           (NextTok->is(tok::hash) && isOnNewLine(*NextTok))) {
-      while (NextTok->is(tok::comment))
+    FormatToken *NextTok;
+    do {
+      NextTok = Tokens->getNextToken();
+    } while (NextTok->is(tok::comment));
+    while (NextTok->is(tok::hash)) {
+      NextTok = Tokens->getNextToken();
+      do {
         NextTok = Tokens->getNextToken();
-      while (NextTok->is(tok::hash) && isOnNewLine(*NextTok)) {
-        ScopedMacroState MacroState(*Line, Tokens, NextTok);
-        do {
-          NextTok = Tokens->getNextToken();
-        } while (NextTok->isNot(tok::eof));
-      }
+      } while (NextTok->is(tok::comment) ||
+               (NextTok->NewlinesBefore == 0 && NextTok->isNot(tok::eof)));
     }
 
     switch (Tok->Tok.getKind()) {