[clang-format] Add ApplyAlwaysOnePerLineToTemplateArguments option #137544
Conversation
|
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. |
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-format Author: Lorenzo Mauro (LorenzoMauro) ChangesIntroduce a new FormatStyle option, This allows users to enforce one-per-line formatting for function parameters without unintentionally splitting template arguments. Includes unit tests covering function declarations, definitions, and template instantiations, both with and without trailing comments. BehaviorIf
template <typename T, int N>
struct Foo {
T mData[N];
Foo<T,
N>
operator+(const Foo<T,
N> &other) const {}
Foo<T,
N>
bar(const Foo<T,
N> &other,
float t) const {}
};
template <typename T, int N>
struct Foo {
T mData[N];
Foo<T, N> operator+(const Foo<T, N> &other) const {}
Foo<T, N> bar(const Foo<T, N> &other,
float t) const {}
};Full diff: https://github.com/llvm/llvm-project/pull/137544.diff 8 Files Affected:
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index f6ceef08b46da..81dafb53b1e89 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -1259,6 +1259,30 @@ struct FormatStyle {
/// \version 3.7
BinPackParametersStyle BinPackParameters;
+ /// If ``BinPackParameters`` is set to ``AlwaysOnePerLine``, specifies whether
+ /// template argument lists should also be split across multiple lines.
+ ///
+ /// When set to ``true``, each template argument will be placed on its own
+ /// line. When set to ``false``, template argument lists remain compact even
+ /// when function parameters are broken one per line.
+ ///
+ /// \code
+ /// true:
+ /// template <
+ /// typename T,
+ /// typename U>
+ /// void foo(
+ /// T a,
+ /// U b);
+ ///
+ /// false:
+ /// template <typename T, typename U>
+ /// void foo(
+ /// T a,
+ /// U b);
+ /// \endcode
+ bool ApplyAlwaysOnePerLineToTemplateArguments = false;
+
/// Styles for adding spacing around ``:`` in bitfield definitions.
enum BitFieldColonSpacingStyle : int8_t {
/// Add one space on each side of the ``:``
@@ -5326,6 +5350,8 @@ struct FormatStyle {
BinPackArguments == R.BinPackArguments &&
BinPackLongBracedList == R.BinPackLongBracedList &&
BinPackParameters == R.BinPackParameters &&
+ ApplyAlwaysOnePerLineToTemplateArguments ==
+ R.ApplyAlwaysOnePerLineToTemplateArguments &&
BitFieldColonSpacing == R.BitFieldColonSpacing &&
BracedInitializerIndentWidth == R.BracedInitializerIndentWidth &&
BreakAdjacentStringLiterals == R.BreakAdjacentStringLiterals &&
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 5a1c3f556b331..13f3bc3db3cdc 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1007,6 +1007,8 @@ template <> struct MappingTraits<FormatStyle> {
IO.mapOptional("BinPackArguments", Style.BinPackArguments);
IO.mapOptional("BinPackLongBracedList", Style.BinPackLongBracedList);
IO.mapOptional("BinPackParameters", Style.BinPackParameters);
+ IO.mapOptional("ApplyAlwaysOnePerLineToTemplateArguments",
+ Style.ApplyAlwaysOnePerLineToTemplateArguments);
IO.mapOptional("BitFieldColonSpacing", Style.BitFieldColonSpacing);
IO.mapOptional("BracedInitializerIndentWidth",
Style.BracedInitializerIndentWidth);
@@ -1521,6 +1523,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
LLVMStyle.BinPackArguments = true;
LLVMStyle.BinPackLongBracedList = true;
LLVMStyle.BinPackParameters = FormatStyle::BPPS_BinPack;
+ LLVMStyle.ApplyAlwaysOnePerLineToTemplateArguments = true;
LLVMStyle.BitFieldColonSpacing = FormatStyle::BFCS_Both;
LLVMStyle.BracedInitializerIndentWidth = -1;
LLVMStyle.BraceWrapping = {/*AfterCaseLabel=*/false,
diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index 946cd7b81587f..7f15c48f15bc3 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -309,11 +309,11 @@ struct FormatToken {
IsUnterminatedLiteral(false), CanBreakBefore(false),
ClosesTemplateDeclaration(false), StartsBinaryExpression(false),
EndsBinaryExpression(false), PartOfMultiVariableDeclStmt(false),
- ContinuesLineCommentSection(false), Finalized(false),
- ClosesRequiresClause(false), EndsCppAttributeGroup(false),
- BlockKind(BK_Unknown), Decision(FD_Unformatted),
- PackingKind(PPK_Inconclusive), TypeIsFinalized(false),
- Type(TT_Unknown) {}
+ InTemplateArgumentList(false), ContinuesLineCommentSection(false),
+ Finalized(false), ClosesRequiresClause(false),
+ EndsCppAttributeGroup(false), BlockKind(BK_Unknown),
+ Decision(FD_Unformatted), PackingKind(PPK_Inconclusive),
+ TypeIsFinalized(false), Type(TT_Unknown) {}
/// The \c Token.
Token Tok;
@@ -373,6 +373,9 @@ struct FormatToken {
/// Only set if \c Type == \c TT_StartOfName.
unsigned PartOfMultiVariableDeclStmt : 1;
+ /// \c true if this token is part of a template argument list.
+ unsigned InTemplateArgumentList : 1;
+
/// Does this line comment continue a line comment section?
///
/// Only set to true if \c Type == \c TT_LineComment.
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index e56cc92987af7..b39c4b2bb609c 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -1977,6 +1977,19 @@ class AnnotatingParser {
return Type;
}
+ void markTokenAsTemplateArgumentInLine() {
+ int TemplateDepth = 0;
+ for (FormatToken *Tok = Line.First; Tok; Tok = Tok->Next) {
+ if (Tok->is(TT_TemplateCloser))
+ --TemplateDepth;
+
+ Tok->InTemplateArgumentList = (TemplateDepth > 0);
+
+ if (Tok->is(TT_TemplateOpener))
+ ++TemplateDepth;
+ }
+ }
+
public:
LineType parseLine() {
if (!CurrentToken)
@@ -2074,6 +2087,7 @@ class AnnotatingParser {
if (ctx.ContextType == Context::StructArrayInitializer)
return LT_ArrayOfStructInitializer;
+ markTokenAsTemplateArgumentInLine();
return LT_Other;
}
@@ -5619,6 +5633,8 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
// BreakFunctionDefinitionParameters or AlignAfterOpenBracket.
if (Style.BinPackParameters == FormatStyle::BPPS_AlwaysOnePerLine &&
Line.MightBeFunctionDecl && !Left.opensScope() &&
+ (Style.ApplyAlwaysOnePerLineToTemplateArguments ||
+ !Left.InTemplateArgumentList) &&
startsNextParameter(Right, Style)) {
return true;
}
diff --git a/clang/lib/Format/TokenAnnotator.h b/clang/lib/Format/TokenAnnotator.h
index e4b94431e68b4..4d72916dc64a7 100644
--- a/clang/lib/Format/TokenAnnotator.h
+++ b/clang/lib/Format/TokenAnnotator.h
@@ -186,6 +186,9 @@ class AnnotatedLine {
bool MightBeFunctionDecl;
bool IsMultiVariableDeclStmt;
+ /// \c True if this token is part o a template declaration.
+ bool InTemplateDecl = false;
+
/// \c True if this line contains a macro call for which an expansion exists.
bool ContainsMacroCall = false;
diff --git a/clang/unittests/Format/ConfigParseTest.cpp b/clang/unittests/Format/ConfigParseTest.cpp
index 2b08b794792e9..31f1cbe8e4bf7 100644
--- a/clang/unittests/Format/ConfigParseTest.cpp
+++ b/clang/unittests/Format/ConfigParseTest.cpp
@@ -482,6 +482,12 @@ TEST(ConfigParseTest, ParsesConfiguration) {
CHECK_PARSE("BinPackParameters: false", BinPackParameters,
FormatStyle::BPPS_OnePerLine);
+ Style.ApplyAlwaysOnePerLineToTemplateArguments = false;
+ CHECK_PARSE("ApplyAlwaysOnePerLineToTemplateArguments: true",
+ ApplyAlwaysOnePerLineToTemplateArguments, true);
+ CHECK_PARSE("ApplyAlwaysOnePerLineToTemplateArguments: false",
+ ApplyAlwaysOnePerLineToTemplateArguments, false);
+
Style.PackConstructorInitializers = FormatStyle::PCIS_BinPack;
CHECK_PARSE("PackConstructorInitializers: Never", PackConstructorInitializers,
FormatStyle::PCIS_Never);
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 333d40d481025..a50e1cc9b3d38 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -9024,6 +9024,55 @@ TEST_F(FormatTest, FormatsDeclarationBreakAlways) {
BreakAlways);
}
+TEST_F(FormatTest, ApplyAlwaysOnePerLineToTemplateArguments) {
+ FormatStyle Style = getGoogleStyle();
+ Style.BinPackParameters = FormatStyle::BPPS_AlwaysOnePerLine;
+
+ // Case 1: Template arguments split by AlwaysOnePerLine
+ Style.ApplyAlwaysOnePerLineToTemplateArguments = true;
+ verifyFormat("template <typename T, int N>\n"
+ "struct Foo {\n"
+ " T mData[N];\n"
+ " Foo<T,\n"
+ " N>\n"
+ " operator+(const Foo<T,\n"
+ " N> &other) const {}\n"
+ " Foo<T,\n"
+ " N>\n"
+ " bar(const Foo<T,\n"
+ " N> &other,\n"
+ " float t) const {}\n"
+ "};\n",
+ Style);
+
+ // Case 2: Template arguments not split by The
+ // ApplyAlwaysOnePerLineToTemplateArguments
+ Style.ApplyAlwaysOnePerLineToTemplateArguments = false;
+ verifyFormat("template <typename T, int N>\n"
+ "struct Foo {\n"
+ " T mData[N];\n"
+ " Foo<T, N> operator+(const Foo<T, N> &other) const {}\n"
+ " Foo<T, N> bar(const Foo<T, N> &other,\n"
+ " float t) const {}\n"
+ "};\n",
+ Style);
+
+ // Case 3: Template arguments not split by the
+ // ApplyAlwaysOnePerLineToTemplateArguments but using the
+ // BreakFunctionDefinitionParameters flag
+ Style.BreakFunctionDefinitionParameters = true;
+ verifyFormat("template <typename T, int N>\n"
+ "struct Foo {\n"
+ " T mData[N];\n"
+ " Foo<T, N> operator+(\n"
+ " const Foo<T, N> &other) const {}\n"
+ " Foo<T, N> bar(\n"
+ " const Foo<T, N> &other,\n"
+ " float t) const {}\n"
+ "};\n",
+ Style);
+}
+
TEST_F(FormatTest, FormatsDefinitionBreakAlways) {
FormatStyle BreakAlways = getGoogleStyle();
BreakAlways.BinPackParameters = FormatStyle::BPPS_AlwaysOnePerLine;
diff --git a/clang/unittests/Format/FormatTestComments.cpp b/clang/unittests/Format/FormatTestComments.cpp
index 5eefd767706a3..9b04a3cfc7553 100644
--- a/clang/unittests/Format/FormatTestComments.cpp
+++ b/clang/unittests/Format/FormatTestComments.cpp
@@ -444,6 +444,66 @@ TEST_F(FormatTestComments, UnderstandsBlockComments) {
" int jjj; /*b*/");
}
+TEST_F(FormatTestComments,
+ AlwaysOnePerLineRespectsTemplateArgumentsFlagWithComments) {
+ FormatStyle Style = getGoogleStyle();
+ Style.BinPackParameters = FormatStyle::BPPS_AlwaysOnePerLine;
+
+ // Case 1: Template arguments split by AlwaysOnePerLine
+ Style.ApplyAlwaysOnePerLineToTemplateArguments = true;
+ verifyFormat("template <typename T, // comment\n"
+ " int N> // comment\n"
+ "struct Foo {\n"
+ " T mData[N];\n"
+ " Foo<T,\n"
+ " N>\n"
+ " operator+(const Foo<T,\n"
+ " N> &other) const { // comment\n"
+ " }\n"
+ " Foo<T,\n"
+ " N>\n"
+ " bar(const Foo<T,\n"
+ " N> &other, // comment\n"
+ " float t) const { // comment\n"
+ " }\n"
+ "};\n",
+ Style);
+
+ // Case 2: Template arguments not split by The
+ // ApplyAlwaysOnePerLineToTemplateArguments
+ Style.ApplyAlwaysOnePerLineToTemplateArguments = false;
+ verifyFormat(
+ "template <typename T, // comment\n"
+ " int N> // comment\n"
+ "struct Foo {\n"
+ " T mData[N];\n"
+ " Foo<T, N> operator+(const Foo<T, N> &other) const { // comment\n"
+ " }\n"
+ " Foo<T, N> bar(const Foo<T, N> &other, // comment\n"
+ " float t) const { // comment\n"
+ " }\n"
+ "};\n",
+ Style);
+
+ // Case 3: Template arguments not split by the
+ // ApplyAlwaysOnePerLineToTemplateArguments but using the
+ // BreakFunctionDefinitionParameters flag
+ Style.BreakFunctionDefinitionParameters = true;
+ verifyFormat("template <typename T, // comment\n"
+ " int N> // comment\n"
+ "struct Foo {\n"
+ " T mData[N];\n"
+ " Foo<T, N> operator+(\n"
+ " const Foo<T, N> &other) const { // comment\n"
+ " }\n"
+ " Foo<T, N> bar(\n"
+ " const Foo<T, N> &other, // comment\n"
+ " float t) const { // comment\n"
+ " }\n"
+ "};\n",
+ Style);
+}
+
TEST_F(FormatTestComments, AlignsBlockComments) {
EXPECT_EQ("/*\n"
" * Really multi-line\n"
|
|
Thanks for taking a look! |
Introduce a new FormatStyle option, ApplyAlwaysOnePerLineToTemplateArguments, which controls whether BinPackParameters=AlwaysOnePerLine also applies to template argument lists. This allows users to enforce one-per-line for function parameters without unintentionally splitting template parameters. Includes unit tests covering both function declarations and definitions, with and without trailing comments.
LorenzoMauro
force-pushed
the
clang-format-custom
branch
from
9baa261 to
b4ea6ee
Compare
| /// }; | ||
| /// \endcode | ||
| /// \version 21 | ||
| bool ApplyAlwaysOnePerLineToTemplateArguments; |
There was a problem hiding this comment.
Please sort alphabetically.
| @@ -1259,6 +1259,45 @@ struct FormatStyle { | |||
| /// \version 3.7 | |||
| BinPackParametersStyle BinPackParameters; | |||
|
|
|||
| /// If ``BinPackParameters`` is set to ``AlwaysOnePerLine``, specifies whether | |||
There was a problem hiding this comment.
Add a reference on BinPackParameters to this one.
| @@ -5326,6 +5365,8 @@ struct FormatStyle { | |||
| BinPackArguments == R.BinPackArguments && | |||
| BinPackLongBracedList == R.BinPackLongBracedList && | |||
| BinPackParameters == R.BinPackParameters && | |||
| ApplyAlwaysOnePerLineToTemplateArguments == | |||
| @@ -2074,6 +2087,7 @@ class AnnotatingParser { | |||
| if (ctx.ContextType == Context::StructArrayInitializer) | |||
| return LT_ArrayOfStructInitializer; | |||
|
|
|||
| markTokenAsTemplateArgumentInLine(); | |||
There was a problem hiding this comment.
I think you want this before the preceding loop. And can you get it into the first pass of the line? Just going through all tokens again doesn't seem nice.
| @@ -186,6 +186,9 @@ class AnnotatedLine { | |||
| bool MightBeFunctionDecl; | |||
| bool IsMultiVariableDeclStmt; | |||
|
|
|||
| /// \c True if this token is part o a template declaration. | |||
| bool InTemplateDecl = false; | |||
| @@ -9024,6 +9024,55 @@ TEST_F(FormatTest, FormatsDeclarationBreakAlways) { | |||
| BreakAlways); | |||
| } | |||
|
|
|||
| TEST_F(FormatTest, ApplyAlwaysOnePerLineToTemplateArguments) { | |||
| FormatStyle Style = getGoogleStyle(); | |||
There was a problem hiding this comment.
Why google style?
| @@ -444,6 +444,66 @@ TEST_F(FormatTestComments, UnderstandsBlockComments) { | |||
| " int jjj; /*b*/"); | |||
| } | |||
|
|
|||
| TEST_F(FormatTestComments, | |||
There was a problem hiding this comment.
What's your reason for this test? You didn't change anything comment related, right?
This looks like a bug. Can we fix it without introducing a new option? If not, can we add it as a sub-option to |
|
Thank you @HazardyKnusperkeks for the review. @owenca I agree — I also don't see why anyone would want the current behavior, so it definitely seems like a bug to me as well. If we take that approach, most of the changes in the PR would also become unnecessary. Let me know how you'd like to proceed, and I’ll update the patch accordingly. |
|
I don't like new options for bug fixes. WDYT @mydeveloperday? |
|
It seems like What about separating it out into I imagine that backwards compatibility could be maintained by having |
| @@ -1521,6 +1523,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) { | |||
| LLVMStyle.BinPackArguments = true; | |||
| LLVMStyle.BinPackLongBracedList = true; | |||
| LLVMStyle.BinPackParameters = FormatStyle::BPPS_BinPack; | |||
| LLVMStyle.ApplyAlwaysOnePerLineToTemplateArguments = true; | |||
There was a problem hiding this comment.
what is the current behavior? is this the correct default?
Introduce a new FormatStyle option,
ApplyAlwaysOnePerLineToTemplateArguments,which controls whether
BinPackParameters=AlwaysOnePerLinealso applies to template argument lists.This allows users to enforce one-per-line formatting for function parameters without unintentionally splitting template arguments.
Includes unit tests covering function declarations, definitions, and template instantiations, both with and without trailing comments.
Behavior
If
BinPackParametersis set toAlwaysOnePerLine, this option controls whether template arguments are split across multiple lines.true, which is the current only behaviour, each template argument is placed on its own line:false, template argument lists remain compact even if function parameters are broken one per line: