Skip to content

Conversation

@JessehMSFT
Copy link

There are two primary scenarios where our team uses block comments in function declarations.

  1. To comment out unused parameters
  2. To provide additional context when passing an unnamed parameter

Clang-format alignment currently breaks when it encounters a block comment.

This PR adds a new AlignBlockComments option which will allow these block comments to be aligned.

@github-actions
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@JessehMSFT JessehMSFT marked this pull request as ready for review September 20, 2024 23:48
@llvmbot
Copy link
Member

llvmbot commented Sep 20, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-format

Author: None (JessehMSFT)

Changes

There are two primary scenarios where our team uses block comments in function declarations.

  1. To comment out unused parameters
  2. To provide additional context when passing an unnamed parameter

Clang-format alignment currently breaks when it encounters a block comment.

This PR adds a new AlignBlockComments option which will allow these block comments to be aligned.


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

5 Files Affected:

  • (modified) clang/include/clang/Format/Format.h (+24-1)
  • (modified) clang/lib/Format/Format.cpp (+15-7)
  • (modified) clang/lib/Format/WhitespaceManager.cpp (+10-1)
  • (modified) clang/unittests/Format/ConfigParseTest.cpp (+37-36)
  • (modified) clang/unittests/Format/FormatTest.cpp (+48-1)
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index d8b62c7652a0f6..f7b6bfffd61314 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -260,12 +260,35 @@ struct FormatStyle {
     ///   bbb >>= 2;
     /// \endcode
     bool PadOperators;
+    /// Only for ``AlignConsecutiveDeclarations``.  Whether block comments
+    /// are aligned in declarations.
+    /// \code
+    ///   true:
+    ///   someLongFunction(int                /*a*/,
+    ///                    bool               b,
+    ///                    const std::string& c)
+    ///
+    ///   const bool ret = someLongFunction(4    /*a*/,
+    ///                                     true /*b*/,
+    ///                                     str  /*c*/);
+    ///
+    ///   false:
+    ///   someLongFunction(int /*a*/,
+    ///                    bool b,
+    ///                    const std::string& c)
+    ///
+    ///   const bool ret = someLongFunction(4 /*a*/,
+    ///                                     true /*b*/,
+    ///                                     str /*c*/);
+    /// \endcode
+    bool AlignBlockComments;
     bool operator==(const AlignConsecutiveStyle &R) const {
       return Enabled == R.Enabled && AcrossEmptyLines == R.AcrossEmptyLines &&
              AcrossComments == R.AcrossComments &&
              AlignCompound == R.AlignCompound &&
              AlignFunctionPointers == R.AlignFunctionPointers &&
-             PadOperators == R.PadOperators;
+             PadOperators == R.PadOperators &&
+             AlignBlockComments == R.AlignBlockComments;
     }
     bool operator!=(const AlignConsecutiveStyle &R) const {
       return !(*this == R);
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index d2463b892fbb96..ab8eadd0171aa6 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -48,39 +48,46 @@ template <> struct MappingTraits<FormatStyle::AlignConsecutiveStyle> {
                 FormatStyle::AlignConsecutiveStyle(
                     {/*Enabled=*/false, /*AcrossEmptyLines=*/false,
                      /*AcrossComments=*/false, /*AlignCompound=*/false,
-                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,
+                     /*AlignBlockComments=*/false}));
     IO.enumCase(Value, "Consecutive",
                 FormatStyle::AlignConsecutiveStyle(
                     {/*Enabled=*/true, /*AcrossEmptyLines=*/false,
                      /*AcrossComments=*/false, /*AlignCompound=*/false,
-                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,
+                     /*AlignBlockComments=*/false}));
     IO.enumCase(Value, "AcrossEmptyLines",
                 FormatStyle::AlignConsecutiveStyle(
                     {/*Enabled=*/true, /*AcrossEmptyLines=*/true,
                      /*AcrossComments=*/false, /*AlignCompound=*/false,
-                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,
+                     /*AlignBlockComments=*/false}));
     IO.enumCase(Value, "AcrossComments",
                 FormatStyle::AlignConsecutiveStyle(
                     {/*Enabled=*/true, /*AcrossEmptyLines=*/false,
                      /*AcrossComments=*/true, /*AlignCompound=*/false,
-                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,
+                     /*AlignBlockComments=*/false}));
     IO.enumCase(Value, "AcrossEmptyLinesAndComments",
                 FormatStyle::AlignConsecutiveStyle(
                     {/*Enabled=*/true, /*AcrossEmptyLines=*/true,
                      /*AcrossComments=*/true, /*AlignCompound=*/false,
-                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,
+                     /*AlignBlockComments=*/false}));
 
     // For backward compatibility.
     IO.enumCase(Value, "true",
                 FormatStyle::AlignConsecutiveStyle(
                     {/*Enabled=*/true, /*AcrossEmptyLines=*/false,
                      /*AcrossComments=*/false, /*AlignCompound=*/false,
-                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,
+                     /*AlignBlockComments=*/false}));
     IO.enumCase(Value, "false",
                 FormatStyle::AlignConsecutiveStyle(
                     {/*Enabled=*/false, /*AcrossEmptyLines=*/false,
                      /*AcrossComments=*/false, /*AlignCompound=*/false,
-                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,
+                     /*AlignBlockComments=*/false}));
   }
 
   static void mapping(IO &IO, FormatStyle::AlignConsecutiveStyle &Value) {
@@ -90,6 +97,7 @@ template <> struct MappingTraits<FormatStyle::AlignConsecutiveStyle> {
     IO.mapOptional("AlignCompound", Value.AlignCompound);
     IO.mapOptional("AlignFunctionPointers", Value.AlignFunctionPointers);
     IO.mapOptional("PadOperators", Value.PadOperators);
+    IO.mapOptional("AlignBlockComments", Value.AlignBlockComments);
   }
 };
 
diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp
index fd4a40a86082e2..62d1cfdcff3c27 100644
--- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -1005,6 +1005,12 @@ void WhitespaceManager::alignConsecutiveTableGenDefinitions() {
                          TT_InheritanceColon);
 }
 
+static bool shouldAlignBlockComment(const FormatToken &Tok) {
+  return Tok.is(TT_BlockComment) && Tok.Previous &&
+         !Tok.Previous->isOneOf(TT_StartOfName, TT_BlockComment) && Tok.Next &&
+         Tok.Next->isOneOf(tok::comma, tok::r_paren, TT_BlockComment);
+}
+
 void WhitespaceManager::alignConsecutiveDeclarations() {
   if (!Style.AlignConsecutiveDeclarations.Enabled)
     return;
@@ -1022,7 +1028,10 @@ void WhitespaceManager::alignConsecutiveDeclarations() {
         if (C.Tok->is(TT_FunctionDeclarationName))
           return true;
         if (C.Tok->isNot(TT_StartOfName))
-          return false;
+          if (!Style.AlignConsecutiveDeclarations.AlignBlockComments ||
+              !shouldAlignBlockComment(*C.Tok)) {
+            return false;
+          }
         if (C.Tok->Previous &&
             C.Tok->Previous->is(TT_StatementAttributeLikeMacro))
           return false;
diff --git a/clang/unittests/Format/ConfigParseTest.cpp b/clang/unittests/Format/ConfigParseTest.cpp
index b8bdfaaa74e10e..745d187dddce50 100644
--- a/clang/unittests/Format/ConfigParseTest.cpp
+++ b/clang/unittests/Format/ConfigParseTest.cpp
@@ -300,49 +300,50 @@ TEST(ConfigParseTest, ParsesConfiguration) {
 #define CHECK_ALIGN_CONSECUTIVE(FIELD)                                         \
   do {                                                                         \
     Style.FIELD.Enabled = true;                                                \
-    CHECK_PARSE(                                                               \
-        #FIELD ": None", FIELD,                                                \
-        FormatStyle::AlignConsecutiveStyle(                                    \
-            {/*Enabled=*/false, /*AcrossEmptyLines=*/false,                    \
-             /*AcrossComments=*/false, /*AlignCompound=*/false,                \
-             /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));        \
-    CHECK_PARSE(                                                               \
-        #FIELD ": Consecutive", FIELD,                                         \
-        FormatStyle::AlignConsecutiveStyle(                                    \
-            {/*Enabled=*/true, /*AcrossEmptyLines=*/false,                     \
-             /*AcrossComments=*/false, /*AlignCompound=*/false,                \
-             /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));        \
-    CHECK_PARSE(                                                               \
-        #FIELD ": AcrossEmptyLines", FIELD,                                    \
-        FormatStyle::AlignConsecutiveStyle(                                    \
-            {/*Enabled=*/true, /*AcrossEmptyLines=*/true,                      \
-             /*AcrossComments=*/false, /*AlignCompound=*/false,                \
-             /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));        \
-    CHECK_PARSE(                                                               \
-        #FIELD ": AcrossEmptyLinesAndComments", FIELD,                         \
-        FormatStyle::AlignConsecutiveStyle(                                    \
-            {/*Enabled=*/true, /*AcrossEmptyLines=*/true,                      \
-             /*AcrossComments=*/true, /*AlignCompound=*/false,                 \
-             /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));        \
+    CHECK_PARSE(#FIELD ": None", FIELD,                                        \
+                FormatStyle::AlignConsecutiveStyle(                            \
+                    {/*Enabled=*/false, /*AcrossEmptyLines=*/false,            \
+                     /*AcrossComments=*/false, /*AlignCompound=*/false,        \
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,   \
+                     /*AlignBlockComments=*/false}));                          \
+    CHECK_PARSE(#FIELD ": Consecutive", FIELD,                                 \
+                FormatStyle::AlignConsecutiveStyle(                            \
+                    {/*Enabled=*/true, /*AcrossEmptyLines=*/false,             \
+                     /*AcrossComments=*/false, /*AlignCompound=*/false,        \
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,   \
+                     /*AlignBlockComments=*/false}));                          \
+    CHECK_PARSE(#FIELD ": AcrossEmptyLines", FIELD,                            \
+                FormatStyle::AlignConsecutiveStyle(                            \
+                    {/*Enabled=*/true, /*AcrossEmptyLines=*/true,              \
+                     /*AcrossComments=*/false, /*AlignCompound=*/false,        \
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,   \
+                     /*AlignBlockComments=*/false}));                          \
+    CHECK_PARSE(#FIELD ": AcrossEmptyLinesAndComments", FIELD,                 \
+                FormatStyle::AlignConsecutiveStyle(                            \
+                    {/*Enabled=*/true, /*AcrossEmptyLines=*/true,              \
+                     /*AcrossComments=*/true, /*AlignCompound=*/false,         \
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,   \
+                     /*AlignBlockComments=*/false}));                          \
     /* For backwards compability, false / true should still parse */           \
-    CHECK_PARSE(                                                               \
-        #FIELD ": false", FIELD,                                               \
-        FormatStyle::AlignConsecutiveStyle(                                    \
-            {/*Enabled=*/false, /*AcrossEmptyLines=*/false,                    \
-             /*AcrossComments=*/false, /*AlignCompound=*/false,                \
-             /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));        \
-    CHECK_PARSE(                                                               \
-        #FIELD ": true", FIELD,                                                \
-        FormatStyle::AlignConsecutiveStyle(                                    \
-            {/*Enabled=*/true, /*AcrossEmptyLines=*/false,                     \
-             /*AcrossComments=*/false, /*AlignCompound=*/false,                \
-             /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));        \
+    CHECK_PARSE(#FIELD ": false", FIELD,                                       \
+                FormatStyle::AlignConsecutiveStyle(                            \
+                    {/*Enabled=*/false, /*AcrossEmptyLines=*/false,            \
+                     /*AcrossComments=*/false, /*AlignCompound=*/false,        \
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,   \
+                     /*AlignBlockComments=*/false}));                          \
+    CHECK_PARSE(#FIELD ": true", FIELD,                                        \
+                FormatStyle::AlignConsecutiveStyle(                            \
+                    {/*Enabled=*/true, /*AcrossEmptyLines=*/false,             \
+                     /*AcrossComments=*/false, /*AlignCompound=*/false,        \
+                     /*AlignFunctionPointers=*/false, /*PadOperators=*/true,   \
+                     /*AlignBlockComments=*/false}));                          \
                                                                                \
     CHECK_PARSE_NESTED_BOOL(FIELD, Enabled);                                   \
     CHECK_PARSE_NESTED_BOOL(FIELD, AcrossEmptyLines);                          \
     CHECK_PARSE_NESTED_BOOL(FIELD, AcrossComments);                            \
     CHECK_PARSE_NESTED_BOOL(FIELD, AlignCompound);                             \
     CHECK_PARSE_NESTED_BOOL(FIELD, PadOperators);                              \
+    CHECK_PARSE_NESTED_BOOL(FIELD, AlignBlockComments);                        \
   } while (false)
 
   CHECK_ALIGN_CONSECUTIVE(AlignConsecutiveAssignments);
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 53aa93a7a4fb01..2f03149609878a 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -20018,6 +20018,52 @@ TEST_F(FormatTest, AlignConsecutiveDeclarations) {
                BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveDeclarationsBlockComments) {
+  FormatStyle Style = getLLVMStyleWithColumns(80);
+  Style.AlignConsecutiveDeclarations.Enabled = true;
+  Style.AlignConsecutiveDeclarations.AlignBlockComments = true;
+  Style.BinPackParameters = FormatStyle::BPPS_OnePerLine;
+  Style.BinPackArguments = false;
+
+  verifyFormat(
+      "bool SomeLongMethodName(int                longParameterNameA,\n"
+      "                        bool               /*longParameterNameB*/,\n"
+      "                        const std::string &longParameterNameC);",
+      "bool SomeLongMethodName(int longParameterNameA,\n"
+      "                        bool /*longParameterNameB*/,\n"
+      "                        const std::string &longParameterNameC);",
+      Style);
+
+  verifyFormat(
+      "const bool ret = SomeLongMethodName(4    /*parameterNameA*/,\n"
+      "                                    true /*longParameterNameB*/,\n"
+      "                                    str  /*longestParameterNameC*/);",
+      "const bool ret = SomeLongMethodName(4 /*parameterNameA*/,\n"
+      "                                    true /*longParameterNameB*/,\n"
+      "                                    str /*longestParameterNameC*/);",
+      Style);
+
+  verifyNoChange("/*,\n"
+                 "This is a multi-line block comment.\n"
+                 "That is not part of a function declaration.\n"
+                 "*/\n"
+                 "static void SomeLongMethodName()",
+                 Style);
+
+  verifyNoChange("static const unsigned int c_cMediaEntranceEntries = 31;\n"
+                 "static const unsigned int c_cMediaEmphasisEntries = 4 /* "
+                 "media effects */ + 12;\n"
+                 "static const unsigned int c_cMediaExitEntries = 32;",
+                 Style);
+
+  verifyNoChange(
+      "static bool SomeLongMethodName(int          longParameterNameA,\n"
+      "                               bool         longParameterNameB "
+      "/*=false*/,\n"
+      "                               std::string &longParameterNameC);",
+      Style);
+}
+
 TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
   FormatStyle Alignment = getLLVMStyle();
   Alignment.AllowShortCaseLabelsOnASingleLine = true;
@@ -20259,7 +20305,8 @@ TEST_F(FormatTest, AlignWithLineBreaks) {
             FormatStyle::AlignConsecutiveStyle(
                 {/*Enabled=*/false, /*AcrossEmptyLines=*/false,
                  /*AcrossComments=*/false, /*AlignCompound=*/false,
-                 /*AlignFunctionPointers=*/false, /*PadOperators=*/true}));
+                 /*AlignFunctionPointers=*/false, /*PadOperators=*/true,
+                 /*AlignBlockComments=*/false}));
   EXPECT_EQ(Style.AlignConsecutiveDeclarations,
             FormatStyle::AlignConsecutiveStyle({}));
   verifyFormat("void foo() {\n"

Copy link
Contributor

@mydeveloperday mydeveloperday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have not regenerated the rst

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Sep 23, 2024
@JessehMSFT
Copy link
Author

You have not regenerated the rst

Thank you for flagging @mydeveloperday, I have regenerated ClangFormatStyleOptions.rst via dump_format_style.py and pushed an update.

@JessehMSFT
Copy link
Author

Ping

@github-actions
Copy link

github-actions bot commented Oct 8, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@JessehMSFT
Copy link
Author

Friendly ping @mydeveloperday

Copy link
Contributor

@owenca owenca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the release notes.

Comment on lines 20049 to 20051
"const bool ret = SomeLongMethodName(4 /*parameterNameA*/,\n"
" true /*longParameterNameB*/,\n"
" str /*longestParameterNameC*/);",
Copy link
Contributor

@owenca owenca Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are arguments and should not be under the new option. This may require its own option.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @owenca , did you have a suggestion? I thought mostly the argument alignment would follow the declaration parameter alignment for consistency.

Comment on lines 1030 to +1034
if (C.Tok->isNot(TT_StartOfName))
return false;
if (!Style.AlignConsecutiveDeclarations.AlignBlockComments ||
!shouldAlignBlockComment(*C.Tok)) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fold the nested if statements or run ninja clang-format-check-format and fix the formatting error below:

--- /Users/Owen/llvm-project/clang/lib/Format/WhitespaceManager.cpp	2025-01-03 21:22:11
+++ -	2025-01-03 22:07:03
@@ -1027,11 +1027,12 @@
         }
         if (C.Tok->is(TT_FunctionDeclarationName))
           return Style.AlignConsecutiveDeclarations.AlignFunctionDeclarations;
-        if (C.Tok->isNot(TT_StartOfName))
+        if (C.Tok->isNot(TT_StartOfName)) {
           if (!Style.AlignConsecutiveDeclarations.AlignBlockComments ||
               !shouldAlignBlockComment(*C.Tok)) {
             return false;
           }
+        }
         if (C.Tok->Previous &&
             C.Tok->Previous->is(TT_StatementAttributeLikeMacro))
           return false;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang Clang issues not falling into any other category clang-format

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants