Skip to content

[clang-format] Allow line breaking with PointerAlignment configured #164686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sstwcw wants to merge 2 commits into from
Closed

[clang-format] Allow line breaking with PointerAlignment configured #164686

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d9d396
[clang-format] Allow line breaking with PointerAlignment configured
sstwcw Oct 22, 2025
3ccc39e
Remove unnecessary test
sstwcw Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion clang/lib/Format/ContinuationIndenter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) &&
Expand Down
6 changes: 4 additions & 2 deletions clang/lib/Format/TokenAnnotator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
32 changes: 32 additions & 0 deletions clang/unittests/Format/FormatTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor

@HazardyKnusperkeks HazardyKnusperkeks Oct 22, 2025

Choose a reason for hiding this comment

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

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);

?

Copy link
Contributor Author

@sstwcw sstwcw Oct 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@HazardyKnusperkeks HazardyKnusperkeks Oct 23, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@owenca owenca Oct 25, 2025

Choose a reason for hiding this comment

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

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?

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.

Copy link
Contributor

@owenca owenca Oct 25, 2025

Choose a reason for hiding this comment

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

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?

Maybe set CanBreakBefore to false if * is followed by a declarator?

Copy link
Contributor

@HazardyKnusperkeks HazardyKnusperkeks Oct 27, 2025

Choose a reason for hiding this comment

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

Type * const arg and Type * const is too long for the ColumnLimit, where to break? We are within the type.

Copy link
Contributor

@owenca owenca Oct 28, 2025

Choose a reason for hiding this comment

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

I'd say break before * because const qualifies it.

"void foo( //\n"
" const MySuperSuperSuperSuperSuperSuperSuperSuperLongTypeName *\n"
" const my_super_super_super_super_long_variable_name) {}",
Comment on lines +8665 to +8666
Copy link
Contributor

@owenca owenca Oct 26, 2025

Choose a reason for hiding this comment

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

Same here.

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"
Expand Down