[clang-format] add option to control bin-packing keyworded parameters #131605
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
| 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 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. |
Member
| @llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-format Author: Eugene Shalygin (zeule) ChangesThe 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:
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 |
| ✅ With the latest revision this PR passed the C/C++ code formatter. |
HazardyKnusperkeks requested changes Mar 17, 2025
Contributor
HazardyKnusperkeks left a comment
HazardyKnusperkeks left a comment There was a problem hiding this comment.
You need to add a parsing test for your option and annotation tests for the annotation.
zeule changed the title zeule force-pushed the feature/format-keyworded-params branch 3 times, most recently from 4f0c149 to 711191c Compare zeule force-pushed the feature/format-keyworded-params branch 3 times, most recently from f5caba3 to 9a5e2d6 Compare zeule force-pushed the feature/format-keyworded-params branch 3 times, most recently from 2c9a741 to 8331579 Compare 
mydeveloperday requested changes Mar 25, 2025
Contributor
mydeveloperday left a comment
mydeveloperday left a comment There was a problem hiding this comment.
Please fix the spellings
Author
I guess the simplest way would be to rename "FunctionDeclarationWithKeywords" (e.g. "KeywordedFunctionDeclaration")? Otherwise I would have to extend the logic in |
zeule force-pushed the feature/format-keyworded-params branch from 8331579 to 4d687d4 Compare Author
| 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. |
Contributor
| Please recheck all your added comments, if the wording is still the best one, after renaming the option. |
zeule force-pushed the feature/format-keyworded-params branch 2 times, most recently from ab0165b to a4b958d Compare zeule force-pushed the feature/format-keyworded-params branch 3 times, most recently from 7642499 to 7d79e05 Compare zeule force-pushed the feature/format-keyworded-params branch from 7d79e05 to 5a1ed72 Compare Contributor
I support going a generic way, even if it currently seems to be only used by Qt. |
zeule force-pushed the feature/format-keyworded-params branch 2 times, most recently from 58eb85b to 352742a Compare HazardyKnusperkeks approved these changes Apr 2, 2025
Contributor
This is very Qt-specific with a peculiar syntax, and there's no published style guide for breaking the property "keywords" (a confusing Qt term IMO), so there's little utility in making it generic now. Besides, we usually start a new option as a boolean and then extend it to an As to the implementation, it can be simplified and probably made less brittle. I would do something like the following instead:
|
Author
| Of course it is only because of the Qt popularity I submit this changeset to the mainline. The keywords are natural delimiters in those declarations, so I can't see the proposed solution to use them as delimiters to be unsuitable to an any significant group of Qt users. In the defense of the generic approach I want to remind that Qt's moc expands macros, thus one can replace the standard keywords (and, I guess, the "Q_PROPERTY" itself) with anything else. I can try to use contexts in the implementation, but I still oppose targeting |
zeule force-pushed the feature/format-keyworded-params branch 2 times, most recently from 96bdc35 to 7019e25 Compare Author
| @owenca, this now uses Context to capture the macro parameters. I hope I correctly understand what you asked for. |
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.
zeule force-pushed the feature/format-keyworded-params branch from 7019e25 to bdc4b9d Compare Contributor
When I said the above in #131605 (comment), I had the following in mind: diff --git a/clang/lib/Format/FormatToken.cpp b/clang/lib/Format/FormatToken.cpp @@ -34,6 +34,17 @@ const char *getTokenTypeName(TokenType Type) { return nullptr; } +static SmallVector<StringRef> QtPropertyKeywords = { + "BINDABLE", "CONSTANT", "DESIGNABLE", "FINAL", "MEMBER", + "NOTIFY", "READ", "REQUIRED", "RESET", "REVISION", + "SCRIPTABLE", "STORED", "USER", "WRITE", +}; + +bool FormatToken::isQtProperty() const { + return std::binary_search(QtPropertyKeywords.begin(), + QtPropertyKeywords.end(), TokenText); +} + // Sorted common C++ non-keyword types. static SmallVector<StringRef> CppNonKeywordTypes = { "clock_t", "int16_t", "int32_t", "int64_t", "int8_t", @@ -331,6 +342,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_QtProperty)) + 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 @@ -135,6 +135,7 @@ namespace format { TYPE(PointerOrReference) \ TYPE(ProtoExtensionLSquare) \ TYPE(PureVirtualSpecifier) \ + TYPE(QtProperty) \ TYPE(RangeBasedForLoopColon) \ TYPE(RecordLBrace) \ TYPE(RecordRBrace) \ @@ -703,6 +704,7 @@ public: isAttribute(); } + [[nodiscard]] bool isQtProperty() const; [[nodiscard]] bool isTypeName(const LangOptions &LangOpts) const; [[nodiscard]] bool isTypeOrIdentifier(const LangOptions &LangOpts) const; diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp @@ -390,6 +390,10 @@ private: OpeningParen.Previous->is(tok::kw__Generic)) { Contexts.back().ContextType = Context::C11GenericSelection; Contexts.back().IsExpression = true; + } else if (OpeningParen.Previous && + OpeningParen.Previous->TokenText == "Q_PROPERTY") { + Contexts.back().ContextType = Context::QtProperty; + Contexts.back().IsExpression = false; } else if (Line.InPPDirective && (!OpeningParen.Previous || OpeningParen.Previous->isNot(tok::identifier))) { @@ -1811,6 +1815,11 @@ private: return false; } } + if (Style.AllowBreakBeforeQtProperty && + Contexts.back().ContextType == Context::QtProperty && + Tok->isQtProperty()) { + Tok->setFinalizedType(TT_QtProperty); + } break; case tok::arrow: if (Tok->isNot(TT_LambdaArrow) && Tok->Previous && @@ -2179,6 +2188,7 @@ private: TemplateArgument, // C11 _Generic selection. C11GenericSelection, + QtProperty, // Like in the outer parentheses in `ffnand ff1(.q());`. VerilogInstancePortList, } ContextType = Unknown; @@ -6235,7 +6245,7 @@ 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_ClassHeadName, TT_QtProperty, tok::kw_operator)) { return true; } if (Right.isAttribute()) |
Author
| Thanks you, @owenca, this code is of course much simpler, but it has little to do with what I wanted to implement. Could, you, please, commit that with your authorship? I guess the tests from this PR can be re-used. |
Author
Sorry, little in common with what I wanted to implement. |
Contributor
Np. See #159909. |
Author
| Thanks! |
Contributor
Author
| Thanks! |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
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.