[clang-format] add option to control bin-packing keyworded parameters

[zeule]

The Q_PROPERTY declaration is almost like a function declaration, but uses keywords as parameter separators. This allows users to provide list of those keywords to be used to control bin-packing of the macro parameters.

[github-actions]

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.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-format

Author: Eugene Shalygin (zeule)

Changes

The Q_PROPERTY declaration is almost like a function declaration, but uses keywords as parameter separators. This allows users to provide list of those keywords to be used to control bin-packing of the macro parameters.

---

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

7 Files Affected:

• (modified) clang/include/clang/Format/Format.h (+39-1)
• (modified) clang/lib/Format/ContinuationIndenter.cpp (+4)
• (modified) clang/lib/Format/Format.cpp (+11)
• (modified) clang/lib/Format/FormatToken.cpp (+2)
• (modified) clang/lib/Format/FormatToken.h (+1)
• (modified) clang/lib/Format/TokenAnnotator.cpp (+50-4)
• (modified) clang/unittests/Format/FormatTest.cpp (+52)

diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index fec47a248abb4..22c385d231a1e 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -5247,6 +5247,41 @@ struct FormatStyle {
   /// \version 20
   WrapNamespaceBodyWithEmptyLinesStyle WrapNamespaceBodyWithEmptyLines;
 
+  struct FunctionDeclarationWithKeywords {
+    std::string Name;
+    std::vector<std::string> Keywords;
+
+    bool operator==(const FunctionDeclarationWithKeywords &Other) const {
+      return Name == Other.Name && Keywords == Other.Keywords;
+    }
+  };
+
+  /// Allows to format function-line macros with keyworded parameters according
+  /// to the BinPackParameters setting, treating keywords as parameter
+  /// sepratators.
+  /// \version 21
+  ///
+  /// Q_PROPERTY is an example of such a macro:
+  /// \code
+  /// Q_PROPERTY(int name READ name WRITE setName NOTIFY nameChanged)
+  /// \endcode
+  ///
+  /// With BinPackParameters set to OnePerLine (or AlwaysOnePerLine) and
+  /// \code
+  /// FunctionDeclarationsWithKeywords:
+  /// - Name: "Q_PROPERTY"
+  ///   Keywords: ['READ', 'WRITE', 'MEMBER', 'RESET', 'NOTIFY']
+  /// \endcode
+  /// the line above will be split on these keywords:
+  /// \code
+  /// Q_PROPERTY(
+  ///     int name
+  ///     READ name
+  ///     WRITE setName
+  ///     NOTIFY nameChanged)
+  /// \endcode
+  std::vector<FunctionDeclarationWithKeywords> FunctionDeclarationsWithKeywords;
+
   bool operator==(const FormatStyle &R) const {
     return AccessModifierOffset == R.AccessModifierOffset &&
            AlignAfterOpenBracket == R.AlignAfterOpenBracket &&
@@ -5435,7 +5470,10 @@ struct FormatStyle {
            VerilogBreakBetweenInstancePorts ==
                R.VerilogBreakBetweenInstancePorts &&
            WhitespaceSensitiveMacros == R.WhitespaceSensitiveMacros &&
-           WrapNamespaceBodyWithEmptyLines == R.WrapNamespaceBodyWithEmptyLines;
+           WrapNamespaceBodyWithEmptyLines ==
+               R.WrapNamespaceBodyWithEmptyLines &&
+           FunctionDeclarationsWithKeywords ==
+               R.FunctionDeclarationsWithKeywords;
   }
 
   std::optional<FormatStyle> GetLanguageStyle(LanguageKind Language) const;
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index 1969f4297b211..37fd70a0fd721 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -349,6 +349,10 @@ bool ContinuationIndenter::canBreak(const LineState &State) {
     }
   }
 
+  // Don't break between function parameter keywords and parameter names
+  if (Previous.is(TT_FunctionParameterKeyword) && Current.is(TT_StartOfName))
+    return false;
+
   // Don't allow breaking before a closing brace of a block-indented braced list
   // initializer if there isn't already a break.
   if (Current.is(tok::r_brace) && Current.MatchingParen &&
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index c67db4752e87b..14690a28970fa 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -29,6 +29,7 @@
 using clang::format::FormatStyle;
 
 LLVM_YAML_IS_SEQUENCE_VECTOR(FormatStyle::RawStringFormat)
+LLVM_YAML_IS_SEQUENCE_VECTOR(FormatStyle::FunctionDeclarationWithKeywords)
 
 namespace llvm {
 namespace yaml {
@@ -852,6 +853,14 @@ struct ScalarEnumerationTraits<
   }
 };
 
+template <> struct MappingTraits<FormatStyle::FunctionDeclarationWithKeywords> {
+  static void mapping(IO &IO,
+                      FormatStyle::FunctionDeclarationWithKeywords &Function) {
+    IO.mapOptional("Name", Function.Name);
+    IO.mapOptional("Keywords", Function.Keywords);
+  }
+};
+
 template <> struct MappingTraits<FormatStyle> {
   static void mapping(IO &IO, FormatStyle &Style) {
     // When reading, read the language first, we need it for getPredefinedStyle.
@@ -1192,6 +1201,8 @@ template <> struct MappingTraits<FormatStyle> {
                    Style.WhitespaceSensitiveMacros);
     IO.mapOptional("WrapNamespaceBodyWithEmptyLines",
                    Style.WrapNamespaceBodyWithEmptyLines);
+    IO.mapOptional("FunctionDeclarationsWithKeywords",
+                   Style.FunctionDeclarationsWithKeywords);
 
     // If AlwaysBreakAfterDefinitionReturnType was specified but
     // BreakAfterReturnType was not, initialize the latter from the former for
diff --git a/clang/lib/Format/FormatToken.cpp b/clang/lib/Format/FormatToken.cpp
index 7752139142430..28a49124cf21f 100644
--- a/clang/lib/Format/FormatToken.cpp
+++ b/clang/lib/Format/FormatToken.cpp
@@ -331,6 +331,8 @@ bool startsNextParameter(const FormatToken &Current, const FormatStyle &Style) {
   }
   if (Style.Language == FormatStyle::LK_Proto && Current.is(TT_SelectorName))
     return true;
+  if (Current.is(TT_FunctionParameterKeyword))
+    return true;
   return Previous.is(tok::comma) && !Current.isTrailingComment() &&
          ((Previous.isNot(TT_CtorInitializerComma) ||
            Style.BreakConstructorInitializers !=
diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index 3808872d227a9..accf21da1c1a9 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -82,6 +82,7 @@ namespace format {
   TYPE(FunctionAnnotationRParen)                                               \
   TYPE(FunctionDeclarationName)                                                \
   TYPE(FunctionDeclarationLParen)                                              \
+  TYPE(FunctionParameterKeyword)                                               \
   TYPE(FunctionLBrace)                                                         \
   TYPE(FunctionLikeOrFreestandingMacro)                                        \
   TYPE(FunctionTypeLParen)                                                     \
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 35577cd6db7a1..be84b71b46ffa 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -116,6 +116,18 @@ static bool isCppAttribute(bool IsCpp, const FormatToken &Tok) {
   return AttrTok && AttrTok->startsSequence(tok::r_square, tok::r_square);
 }
 
+static bool isParametersKeyword(
+    const FormatToken &Tok,
+    const FormatStyle::FunctionDeclarationWithKeywords *declaration) {
+  if (!declaration)
+    return false;
+
+  for (auto &Keyword : declaration->Keywords)
+    if (Tok.TokenText == Keyword)
+      return true;
+  return false;
+}
+
 /// A parser that gathers additional information about tokens.
 ///
 /// The \c TokenAnnotator tries to match parenthesis and square brakets and
@@ -148,6 +160,24 @@ class AnnotatingParser {
     }
   }
 
+  const FormatStyle::FunctionDeclarationWithKeywords *
+  isInsideFunctionWithKeywordedParameters(const FormatToken &Token) const {
+    const FormatToken *Previous = &Token;
+    while (auto Prev = Previous->getPreviousNonComment())
+      Previous = Prev;
+    // Unknown if line ends with ';', FunctionLikeOrFreestandingMacro otherwise
+    if (!Previous->isOneOf(TT_FunctionLikeOrFreestandingMacro, TT_Unknown))
+      return nullptr;
+    auto it = std::find_if(
+        Style.FunctionDeclarationsWithKeywords.begin(),
+        Style.FunctionDeclarationsWithKeywords.end(),
+        [Previous](
+            FormatStyle::FunctionDeclarationWithKeywords const &declaration) {
+          return Previous->TokenText == declaration.Name;
+        });
+    return it != Style.FunctionDeclarationsWithKeywords.end() ? &*it : nullptr;
+  }
+
   bool parseAngle() {
     if (!CurrentToken)
       return false;
@@ -2416,8 +2446,13 @@ class AnnotatingParser {
       Current.setType(TT_BinaryOperator);
     } else if (isStartOfName(Current) &&
                (!Line.MightBeFunctionDecl || Current.NestingLevel != 0)) {
-      Contexts.back().FirstStartOfName = &Current;
-      Current.setType(TT_StartOfName);
+      if (isParametersKeyword(
+              Current, isInsideFunctionWithKeywordedParameters(Current))) {
+        Current.setType(TT_FunctionParameterKeyword);
+      } else {
+        Contexts.back().FirstStartOfName = &Current;
+        Current.setType(TT_StartOfName);
+      }
     } else if (Current.is(tok::semi)) {
       // Reset FirstStartOfName after finding a semicolon so that a for loop
       // with multiple increment statements is not confused with a for loop
@@ -3784,10 +3819,20 @@ void TokenAnnotator::annotate(AnnotatedLine &Line) {
 static bool isFunctionDeclarationName(const LangOptions &LangOpts,
                                       const FormatToken &Current,
                                       const AnnotatedLine &Line,
+                                      const FormatStyle &Style,
                                       FormatToken *&ClosingParen) {
   if (Current.is(TT_FunctionDeclarationName))
     return true;
 
+  if (Current.is(TT_FunctionLikeOrFreestandingMacro) &&
+      std::find_if(
+          Style.FunctionDeclarationsWithKeywords.begin(),
+          Style.FunctionDeclarationsWithKeywords.end(),
+          [&Current](const FormatStyle::FunctionDeclarationWithKeywords &Decl) {
+            return Current.TokenText == Decl.Name;
+          }) != Style.FunctionDeclarationsWithKeywords.end()) {
+    return true;
+  }
   if (!Current.Tok.getIdentifierInfo())
     return false;
 
@@ -3994,7 +4039,7 @@ void TokenAnnotator::calculateFormattingInformation(AnnotatedLine &Line) const {
       AfterLastAttribute = Tok;
     if (const bool IsCtorOrDtor = Tok->is(TT_CtorDtorDeclName);
         IsCtorOrDtor ||
-        isFunctionDeclarationName(LangOpts, *Tok, Line, ClosingParen)) {
+        isFunctionDeclarationName(LangOpts, *Tok, Line, Style, ClosingParen)) {
       if (!IsCtorOrDtor)
         Tok->setFinalizedType(TT_FunctionDeclarationName);
       LineIsFunctionDeclaration = true;
@@ -6218,7 +6263,8 @@ bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
               Right.Next->isOneOf(TT_FunctionDeclarationName, tok::kw_const)));
   }
   if (Right.isOneOf(TT_StartOfName, TT_FunctionDeclarationName,
-                    TT_ClassHeadName, tok::kw_operator)) {
+                    TT_FunctionParameterKeyword, TT_ClassHeadName,
+                    tok::kw_operator)) {
     return true;
   }
   if (Right.isAttribute())
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 5df7865f5a629..c08fc7d6c7fc5 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -29096,6 +29096,58 @@ TEST_F(FormatTest, BreakBeforeClassName) {
                "    ArenaSafeUniquePtr {};");
 }
 
+TEST_F(FormatTest, FunctionDeclarationWithKeywords) {
+  FormatStyle::FunctionDeclarationWithKeywords QPropertyDeclaration;
+  QPropertyDeclaration.Name = "Q_PROPERTY";
+  QPropertyDeclaration.Keywords.push_back("READ");
+  QPropertyDeclaration.Keywords.push_back("WRITE");
+  QPropertyDeclaration.Keywords.push_back("NOTIFY");
+  QPropertyDeclaration.Keywords.push_back("RESET");
+
+  auto Style40 = getLLVMStyleWithColumns(40);
+  Style40.FunctionDeclarationsWithKeywords.push_back(QPropertyDeclaration);
+  Style40.BinPackParameters = FormatStyle::BPPS_OnePerLine;
+
+  verifyFormat("Q_PROPERTY(int name\n"
+               "           READ name\n"
+               "           WRITE setName\n"
+               "           NOTIFY nameChanged)",
+               "Q_PROPERTY(int name READ name WRITE setName NOTIFY nameChanged)",
+               Style40);
+  verifyFormat("class A {\n"
+               "  Q_PROPERTY(int name\n"
+               "             READ name\n"
+               "             WRITE setName\n"
+               "             NOTIFY nameChanged)\n"
+               "};",
+               "class A { Q_PROPERTY(int name READ name WRITE setName NOTIFY nameChanged) };",
+               Style40);
+
+  auto Style120 = getLLVMStyleWithColumns(120);
+  Style120.FunctionDeclarationsWithKeywords.push_back(QPropertyDeclaration);
+  Style120.BinPackParameters = FormatStyle::BPPS_AlwaysOnePerLine;
+
+  verifyFormat("Q_PROPERTY(int name\n"
+               "           READ name\n"
+               "           WRITE setName\n"
+               "           NOTIFY nameChanged)",
+               "Q_PROPERTY(int name READ name WRITE setName NOTIFY nameChanged)",
+               Style120);
+  verifyFormat("class A {\n"
+               "  Q_PROPERTY(int name\n"
+               "             READ name\n"
+               "             WRITE setName\n"
+               "             NOTIFY nameChanged)\n"
+               "};",
+               "class A { Q_PROPERTY(int name READ name WRITE setName NOTIFY nameChanged) };",
+               Style120);
+
+  Style120.BinPackParameters = FormatStyle::BPPS_BinPack;
+
+  verifyFormat("Q_PROPERTY(int name READ name WRITE setName NOTIFY nameChanged)",
+               Style120);
+}
+
 } // namespace
 } // namespace test
 } // namespace format

[github-actions]

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

[HazardyKnusperkeks]

You need to add a parsing test for your option and annotation tests for the annotation.

[mydeveloperday]

Please fix the spellings

[zeule]

Please fix the spellings

I guess the simplest way would be to rename "FunctionDeclarationWithKeywords" (e.g. "KeywordedFunctionDeclaration")? Otherwise I would have to extend the logic in pluralize() inside clang/docs/tools/dump_format_style.py?

[zeule]

I renamed the thing so that it ends with a singular noun, which allows the exiting machinery in dump_format_style.py to work, and I like the new name better.

[HazardyKnusperkeks]

Please recheck all your added comments, if the wording is still the best one, after renaming the option.

[HazardyKnusperkeks]

Nearly done. ;)

[zeule]

I hope it is clean now.

[HazardyKnusperkeks]

FAIL: Clang :: Format/docs_updated.test (12079 of 22139)

You forgot to redo the documentation.

[zeule]

Thank you, @HazardyKnusperkeks !

[owenca]

It doesn't seem to make sense to annotate Q_Property as TT_FunctionDeclarationName. Why is this necessary?