[clang] [clang-format] Fix bad comment indentation before ifdef after braceless if (PR #94776)

Erich Reitz via cfe-commits cfe-commits at lists.llvm.org
Fri Jun 7 10:52:33 PDT 2024


https://github.com/Erich-Reitz created https://github.com/llvm/llvm-project/pull/94776

Addresses issue #45002. 

When `readToken()` sees a preprocessor directive, it eagerly flushes the comments.  This changes the behavior if it is within a braceless block.

Keeps behavior of 
```
void f() {
  if (foo) 
    // test
#ifdef
  bax(); 
#endif
}
``` 
formatted as 
```
void f() {
  if (foo) 
  // test
#ifdef
  bax(); 
#endif
}
```
for (specifically) comments within the braceless block 

>From 6c910c8b40be79e3d573f6953860f60ebd27b39f Mon Sep 17 00:00:00 2001
From: Erich Reitz <erich.reitz at protonmail.com>
Date: Fri, 7 Jun 2024 13:04:33 -0400
Subject: [PATCH 1/3] delay flushing comments before ifdef after braceless if;
 align with token that begins conditional

---
 clang/lib/Format/UnwrappedLineParser.cpp | 19 +++++++++++++++----
 clang/lib/Format/UnwrappedLineParser.h   |  2 ++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index b15a87327240b..7bc066787bf46 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -354,6 +354,7 @@ bool UnwrappedLineParser::precededByCommentOrPPDirective() const {
 bool UnwrappedLineParser::parseLevel(const FormatToken *OpeningBrace,
                                      IfStmtKind *IfKind,
                                      FormatToken **IfLeftBrace) {
+
   const bool InRequiresExpression =
       OpeningBrace && OpeningBrace->is(TT_RequiresExpressionLBrace);
   const bool IsPrecededByCommentOrPPDirective =
@@ -385,6 +386,7 @@ bool UnwrappedLineParser::parseLevel(const FormatToken *OpeningBrace,
     };
 
     switch (Kind) {
+
     case tok::comment:
       nextToken();
       addUnwrappedLine();
@@ -1419,6 +1421,7 @@ void UnwrappedLineParser::readTokenWithJavaScriptASI() {
 void UnwrappedLineParser::parseStructuralElement(
     const FormatToken *OpeningBrace, IfStmtKind *IfKind,
     FormatToken **IfLeftBrace, bool *HasDoWhile, bool *HasLabel) {
+
   if (Style.Language == FormatStyle::LK_TableGen &&
       FormatTok->is(tok::pp_include)) {
     nextToken();
@@ -1696,6 +1699,7 @@ void UnwrappedLineParser::parseStructuralElement(
 
   const bool InRequiresExpression =
       OpeningBrace && OpeningBrace->is(TT_RequiresExpressionLBrace);
+
   do {
     const FormatToken *Previous = FormatTok->Previous;
     switch (FormatTok->Tok.getKind()) {
@@ -2705,6 +2709,7 @@ void UnwrappedLineParser::parseUnbracedBody(bool CheckEOF) {
 
   addUnwrappedLine();
   ++Line->Level;
+  ++UnBracedBodyDepth;
   parseStructuralElement();
 
   if (Tok) {
@@ -2719,11 +2724,11 @@ void UnwrappedLineParser::parseUnbracedBody(bool CheckEOF) {
     assert(Tok);
     ++Tok->BraceCount;
   }
-
   if (CheckEOF && eof())
     addUnwrappedLine();
 
   --Line->Level;
+  --UnBracedBodyDepth;
 }
 
 static void markOptionalBraces(FormatToken *LeftBrace) {
@@ -4736,6 +4741,7 @@ void UnwrappedLineParser::distributeComments(
   // the two lines about b form a maximal trail, so there are two sections, the
   // first one consisting of the single comment "// line about a" and the
   // second one consisting of the next two comments.
+
   if (Comments.empty())
     return;
   bool ShouldPushCommentsInCurrentLine = true;
@@ -4811,8 +4817,10 @@ void UnwrappedLineParser::readToken(int LevelDifference) {
            (!Style.isVerilog() ||
             Keywords.isVerilogPPDirective(*Tokens->peekNextToken())) &&
            FirstNonCommentOnLine) {
-      distributeComments(Comments, FormatTok);
-      Comments.clear();
+      if (!UnBracedBodyDepth) {
+        distributeComments(Comments, FormatTok);
+        Comments.clear();
+      }
       // If there is an unfinished unwrapped line, we flush the preprocessor
       // directives only after that unwrapped line was finished later.
       bool SwitchToPreprocessorLines = !Line->Tokens.empty();
@@ -4828,7 +4836,10 @@ void UnwrappedLineParser::readToken(int LevelDifference) {
           PPBranchLevel > 0) {
         Line->Level += PPBranchLevel;
       }
-      flushComments(isOnNewLine(*FormatTok));
+      if (!UnBracedBodyDepth) {
+        flushComments(isOnNewLine(*FormatTok));
+      }
+
       parsePPDirective();
       PreviousWasComment = FormatTok->is(tok::comment);
       FirstNonCommentOnLine = IsFirstNonCommentOnLine(
diff --git a/clang/lib/Format/UnwrappedLineParser.h b/clang/lib/Format/UnwrappedLineParser.h
index d7963a4211bb9..4d87896870a3e 100644
--- a/clang/lib/Format/UnwrappedLineParser.h
+++ b/clang/lib/Format/UnwrappedLineParser.h
@@ -338,6 +338,8 @@ class UnwrappedLineParser {
   // `decltype(auto)`.
   bool IsDecltypeAutoFunction = false;
 
+  int UnBracedBodyDepth = 0;
+
   // Represents preprocessor branch type, so we can find matching
   // #if/#else/#endif directives.
   enum PPBranchKind {

>From b9d52022e1caf314cb3f24f03775c8baf5da1c4a Mon Sep 17 00:00:00 2001
From: Erich Reitz <erich.reitz at protonmail.com>
Date: Fri, 7 Jun 2024 13:08:13 -0400
Subject: [PATCH 2/3] whitespace formatting

---
 clang/lib/Format/UnwrappedLineParser.cpp | 7 +++----
 clang/lib/Format/UnwrappedLineParser.h   | 1 +
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 7bc066787bf46..b17ef33f95e98 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -354,7 +354,6 @@ bool UnwrappedLineParser::precededByCommentOrPPDirective() const {
 bool UnwrappedLineParser::parseLevel(const FormatToken *OpeningBrace,
                                      IfStmtKind *IfKind,
                                      FormatToken **IfLeftBrace) {
-
   const bool InRequiresExpression =
       OpeningBrace && OpeningBrace->is(TT_RequiresExpressionLBrace);
   const bool IsPrecededByCommentOrPPDirective =
@@ -386,7 +385,6 @@ bool UnwrappedLineParser::parseLevel(const FormatToken *OpeningBrace,
     };
 
     switch (Kind) {
-
     case tok::comment:
       nextToken();
       addUnwrappedLine();
@@ -1421,7 +1419,6 @@ void UnwrappedLineParser::readTokenWithJavaScriptASI() {
 void UnwrappedLineParser::parseStructuralElement(
     const FormatToken *OpeningBrace, IfStmtKind *IfKind,
     FormatToken **IfLeftBrace, bool *HasDoWhile, bool *HasLabel) {
-
   if (Style.Language == FormatStyle::LK_TableGen &&
       FormatTok->is(tok::pp_include)) {
     nextToken();
@@ -1699,7 +1696,6 @@ void UnwrappedLineParser::parseStructuralElement(
 
   const bool InRequiresExpression =
       OpeningBrace && OpeningBrace->is(TT_RequiresExpressionLBrace);
-
   do {
     const FormatToken *Previous = FormatTok->Previous;
     switch (FormatTok->Tok.getKind()) {
@@ -2724,6 +2720,7 @@ void UnwrappedLineParser::parseUnbracedBody(bool CheckEOF) {
     assert(Tok);
     ++Tok->BraceCount;
   }
+
   if (CheckEOF && eof())
     addUnwrappedLine();
 
@@ -4817,6 +4814,7 @@ void UnwrappedLineParser::readToken(int LevelDifference) {
            (!Style.isVerilog() ||
             Keywords.isVerilogPPDirective(*Tokens->peekNextToken())) &&
            FirstNonCommentOnLine) {
+
       if (!UnBracedBodyDepth) {
         distributeComments(Comments, FormatTok);
         Comments.clear();
@@ -4836,6 +4834,7 @@ void UnwrappedLineParser::readToken(int LevelDifference) {
           PPBranchLevel > 0) {
         Line->Level += PPBranchLevel;
       }
+
       if (!UnBracedBodyDepth) {
         flushComments(isOnNewLine(*FormatTok));
       }
diff --git a/clang/lib/Format/UnwrappedLineParser.h b/clang/lib/Format/UnwrappedLineParser.h
index 4d87896870a3e..428c7dde182e1 100644
--- a/clang/lib/Format/UnwrappedLineParser.h
+++ b/clang/lib/Format/UnwrappedLineParser.h
@@ -338,6 +338,7 @@ class UnwrappedLineParser {
   // `decltype(auto)`.
   bool IsDecltypeAutoFunction = false;
 
+
   int UnBracedBodyDepth = 0;
 
   // Represents preprocessor branch type, so we can find matching

>From d5aca6de9f513ce049c337dc4c17ea32bb2721aa Mon Sep 17 00:00:00 2001
From: Erich Reitz <erich.reitz at protonmail.com>
Date: Fri, 7 Jun 2024 13:33:28 -0400
Subject: [PATCH 3/3] add basic unit test for change

---
 clang/unittests/Format/FormatTestComments.cpp | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/clang/unittests/Format/FormatTestComments.cpp b/clang/unittests/Format/FormatTestComments.cpp
index d2baace6a7d80..e055a7c59f400 100644
--- a/clang/unittests/Format/FormatTestComments.cpp
+++ b/clang/unittests/Format/FormatTestComments.cpp
@@ -796,6 +796,24 @@ TEST_F(FormatTestComments, ParsesCommentsAdjacentToPPDirectives) {
             format("namespace {}\n   /* Test */    #define A"));
 }
 
+
+TEST_F(FormatTestComments, DeIdentsCommentBeforeIfdefAfterBracelessIf) {
+  EXPECT_EQ("void f() {\n"
+            "  if (true)\n"
+            "    int i;\n"
+            "  /* comment */\n"
+            "#ifdef A\n"
+            "  int j;\n"
+            "}",
+            format("void f() {\n"
+                   "  if (true)\n"
+                   "    int i;\n"
+                   "    /* comment */\n"
+                   "#ifdef A\n"
+                   "  int j;\n"
+                   "}"));
+}
+
 TEST_F(FormatTestComments, KeepsLevelOfCommentBeforePPDirective) {
   // Keep the current level if the comment was originally not aligned with
   // the preprocessor directive.



More information about the cfe-commits mailing list