[clang-format] Allow line breaking with PointerAlignment configured #164686
Conversation
Fixes llvm#139514. Within a long declaration involving the pointer, the program has allowed breaking between the type and the star when the configuration specified that the star should stick to the variable. Now it also allows breaking between the star and the variable when the configuration specified that the star should stick to the type.
|
@llvm/pr-subscribers-clang-format Author: None (sstwcw) ChangesFixes #139514. Within a long declaration involving the pointer, the program has allowed breaking between the type and the star when the configuration specified that the star should stick to the variable. Now it also allows breaking between the star and the variable when the configuration specified that the star should stick to the type. Full diff: https://github.com/llvm/llvm-project/pull/164686.diff 3 Files Affected:
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index e5abf833194d4..88e80349ad31c 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -1566,7 +1566,8 @@ unsigned ContinuationIndenter::getNewLineColumn(const LineState &State) {
}
if (NextNonComment->isOneOf(TT_StartOfName, TT_PointerOrReference) ||
- Previous.isOneOf(tok::coloncolon, tok::equal, TT_JsTypeColon)) {
+ Previous.isOneOf(TT_PointerOrReference, tok::coloncolon, tok::equal,
+ TT_JsTypeColon)) {
return ContinuationIndent;
}
if (PreviousNonComment && PreviousNonComment->is(tok::colon) &&
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 25971d2497f97..3ff5587ab2232 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -4343,7 +4343,7 @@ unsigned TokenAnnotator::splitPenalty(const AnnotatedLine &Line,
return Style.PenaltyReturnTypeOnItsOwnLine;
return 200;
}
- if (Right.is(TT_PointerOrReference))
+ if (Left.is(TT_PointerOrReference) || Right.is(TT_PointerOrReference))
return 190;
if (Right.is(TT_LambdaArrow))
return 110;
@@ -6262,8 +6262,10 @@ bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
TT_ClassHeadName, TT_QtProperty, tok::kw_operator)) {
return true;
}
+ // It is fine to break the line when the * or & should be separate from the
+ // name according to the PointerAlignment config.
if (Left.is(TT_PointerOrReference))
- return false;
+ return Right.SpacesRequiredBefore;
if (Right.isTrailingComment()) {
// We rely on MustBreakBefore being set correctly here as we should not
// change the "binding" behavior of a comment.
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index ce68f91bef02a..e9a95be9a5358 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -8644,6 +8644,38 @@ TEST_F(FormatTest, BreaksFunctionDeclarations) {
" aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}",
Style);
+ Style.ColumnLimit = 70;
+ verifyFormat(
+ "void foo( //\n"
+ " const MySuperSuperSuperSuperSuperSuperSuperSuperLongTypeName*\n"
+ " const my_super_super_super_super_long_variable_name) {}",
+ Style);
+ verifyFormat(
+ "void foo(const MySuperSuperSuperSuperSuperSuperSuperSuperLongTypeName*\n"
+ " my_super_super_super_super_long_variable_name) {}",
+ Style);
+ verifyFormat(
+ "void foo(const MySuperSuperSuperSuperSuperSuperSuperSuperLongTypeName*\n"
+ " const my_super_super_super_super_long_variable_name) {}",
+ Style);
+
+ Style.PointerAlignment = FormatStyle::PAS_Middle;
+ verifyFormat(
+ "void foo( //\n"
+ " const MySuperSuperSuperSuperSuperSuperSuperSuperLongTypeName *\n"
+ " const my_super_super_super_super_long_variable_name) {}",
+ Style);
+ verifyFormat(
+ "void foo(\n"
+ " const MySuperSuperSuperSuperSuperSuperSuperSuperLongTypeName *\n"
+ " my_super_super_super_super_long_variable_name) {}",
+ Style);
+ verifyFormat(
+ "void foo(\n"
+ " const MySuperSuperSuperSuperSuperSuperSuperSuperLongTypeName *\n"
+ " const my_super_super_super_super_long_variable_name) {}",
+ Style);
+
Style = getLLVMStyleWithColumns(45);
Style.PenaltyReturnTypeOnItsOwnLine = 400;
verifyFormat("template <bool abool, // a comment\n"
|
| Style); | ||
|
|
||
| Style.PointerAlignment = FormatStyle::PAS_Middle; | ||
| verifyFormat( |
There was a problem hiding this comment.
Shouldn't it be possible to wrap the * now too? Maybe add a test for that? And what happens if the type and the name are both too long to fit * within the same line, do we get something like
void foo(
Loooooooong
*
looooooong);?
There was a problem hiding this comment.
I looked into it. Now I can't figure out whether the formatter should break the line between the type and the star when middle is configured. I thought that the default right style was for people who read "the expression *x is of type int", while the left and middle styles were for people who read "the variable x is of type int*". Then it seems that the program should avoid breaking between the type and the star when the style is left or middle. However, the code says that the program should break the line between the *const part and the variable when right is selected (pull request #128817, bug report #28919). That seems to contradict my understanding about the styles. What is the reasoning behind the pull request?
There was a problem hiding this comment.
I don't know for whom middle is, but never break between the type and the *. And I think #28919 makes a good point, the const belongs to the type.
On the other hand violating the column limit, while there is whitespace which can be a line break is also bad. I don't really know. Maybe add a huge penalty on that?
@owenca any opinion?
There was a problem hiding this comment.
I looked into it. Now I can't figure out whether the formatter should break the line between the type and the star when middle is configured. I thought that the default right style was for people who read "the expression
*xis of typeint", while the left and middle styles were for people who read "the variablexis of typeint*". Then it seems that the program should avoid breaking between the type and the star when the style is left or middle. However, the code says that the program should break the line between the*constpart and the variable when right is selected (pull request #128817, bug report #28919). That seems to contradict my understanding about the styles. What is the reasoning behind the pull request?
The TT_PointerOrReference token (*, &, or &&) before a declarator is part of the type and should not go with the declarator, so wrapping before * doesn't make sense IMO.
There was a problem hiding this comment.
I don't know for whom middle is, but never break between the type and the
*. And I think #28919 makes a good point, theconstbelongs to the type. On the other hand violating the column limit, while there is whitespace which can be a line break is also bad. I don't really know. Maybe add a huge penalty on that? @owenca any opinion?
Maybe set CanBreakBefore to false if * is followed by a declarator?
There was a problem hiding this comment.
Type * const arg and Type * const is too long for the ColumnLimit, where to break? We are within the type.
There was a problem hiding this comment.
I'd say break before * because const qualifies it.
| "void foo(const MySuperSuperSuperSuperSuperSuperSuperSuperLongTypeName*\n" | ||
| " const my_super_super_super_super_long_variable_name) {}", |
There was a problem hiding this comment.
* and const should stay together on the same line.
There was a problem hiding this comment.
The bug report says that they should be on separate lines because they can't fit on the same line. I don't see how to satisfy the requirement. Please post the formatted code.
| " const MySuperSuperSuperSuperSuperSuperSuperSuperLongTypeName *\n" | ||
| " const my_super_super_super_super_long_variable_name) {}", |
Do I get it right? The program should prefer breaking after the |
|
Why should the void foo(const int (*const (x));That is, why should one think that the second |
4 participants
Fixes #139514.
Within a long declaration involving the pointer, the program has allowed breaking between the type and the star when the configuration specified that the star should stick to the variable. Now it also allows breaking between the star and the variable when the configuration specified that the star should stick to the type.