[clang-format] Improve BlockIndent at ColumnLimit #93140
Merged
Conversation
This file contains 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
|
@llvm/pr-subscribers-clang-format Author: Gedare Bloom (gedare) ChangesFixes #55731 The reported formatting problems were related to ignoring deep nesting of "simple" functions (causing #54808) and to allowing the trailing annotation to become separated from the closing parens, which allowed a break to occur between the closing parens and the trailing annotation. The fix for the nesting of "simple" functions is to detect them more carefully. "Simple" was defined in a comment as being a single non-expression argument. I tried to stay as close to the original intent of the implementation while fixing the various bad formatting reports. In the process of fixing these bugs, some latent bugs were discovered related to how JavaScript Template Strings are handled. Those are also fixed here. Full diff: https://github.com/llvm/llvm-project/pull/93140.diff 3 Files Affected:
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index 6b9fbfe0ebf53..b0a232e97b314 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -803,6 +803,46 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
return !Tok.Previous->isOneOf(TT_CastRParen, tok::kw_for, tok::kw_while,
tok::kw_switch);
};
+ // Detecting functions is brittle. It would be better if we could annotate
+ // the LParen type of functions/calls.
+ const auto IsFunctionDeclParen = [&](const FormatToken &Tok) {
+ return Tok.is(tok::l_paren) && Tok.Previous &&
+ (Tok.Previous->is(TT_FunctionDeclarationName) ||
+ (Tok.Previous->Previous &&
+ Tok.Previous->Previous->is(tok::coloncolon) &&
+ Tok.Previous->Previous->Previous &&
+ Tok.Previous->Previous->Previous->is(TT_FunctionDeclarationName)));
+ };
+ const auto IsFunctionCallParen = [&](const FormatToken &Tok) {
+ return Tok.is(tok::l_paren) && Tok.ParameterCount > 0 && Tok.Previous &&
+ Tok.Previous->is(tok::identifier);
+ };
+ const auto IsInTemplateString = [&](const FormatToken &Tok) {
+ if (!Style.isJavaScript())
+ return false;
+ for (const FormatToken *Prev = &Tok; Prev; Prev = Prev->Previous) {
+ if (Prev->is(TT_TemplateString) && Prev->opensScope())
+ return true;
+ if (Prev->is(TT_TemplateString) && Prev->closesScope())
+ break;
+ }
+ return false;
+ };
+ const auto IsNotSimpleFunction = [&](const FormatToken &Tok) {
+ const auto *Previous = Tok.Previous;
+ const auto *Next = Tok.Next;
+ if (Tok.FakeLParens.size() > 0 && Tok.FakeLParens.back() > prec::Unknown)
+ return true;
+ if (Previous &&
+ (IsFunctionDeclParen(*Previous) || IsFunctionCallParen(*Previous))) {
+ if (!IsOpeningBracket(Tok) && Next && !Next->isMemberAccess() &&
+ !IsInTemplateString(Tok) && !IsFunctionDeclParen(*Next) &&
+ !IsFunctionCallParen(*Next)) {
+ return true;
+ }
+ }
+ return false;
+ };
if ((Style.AlignAfterOpenBracket == FormatStyle::BAS_AlwaysBreak ||
Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent) &&
IsOpeningBracket(Previous) && State.Column > getNewLineColumn(State) &&
@@ -813,10 +853,10 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
// caaaaaaaaaaaall(
// caaaaaaaaaaaall(
// caaaaaaaaaaaaaaaaaaaaaaall(aaaaaaaaaaaaaa, aaaaaaaaa))));
- Current.FakeLParens.size() > 0 &&
- Current.FakeLParens.back() > prec::Unknown) {
+ IsNotSimpleFunction(Current)) {
CurrentState.NoLineBreak = true;
}
+
if (Previous.is(TT_TemplateString) && Previous.opensScope())
CurrentState.NoLineBreak = true;
@@ -831,7 +871,8 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
Previous.isNot(TT_TableGenDAGArgOpenerToBreak) &&
!(Current.MacroParent && Previous.MacroParent) &&
(Current.isNot(TT_LineComment) ||
- Previous.isOneOf(BK_BracedInit, TT_VerilogMultiLineListLParen))) {
+ Previous.isOneOf(BK_BracedInit, TT_VerilogMultiLineListLParen)) &&
+ !IsInTemplateString(Current)) {
CurrentState.Indent = State.Column + Spaces;
CurrentState.IsAligned = true;
}
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 7c4c76a91f2c5..094e87db2426a 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -6157,6 +6157,12 @@ bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
return !(Previous && (Previous->is(tok::kw_for) || Previous->isIf()));
}
+ if (Left.isOneOf(tok::r_paren, TT_TrailingAnnotation) &&
+ Right.is(TT_TrailingAnnotation) &&
+ Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent) {
+ return false;
+ }
+
// Allow breaking after a trailing annotation, e.g. after a method
// declaration.
if (Left.is(TT_TrailingAnnotation)) {
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index a9df994189f00..0413f1f08fd9c 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -9305,6 +9305,28 @@ TEST_F(FormatTest, AlignsAfterOpenBracket) {
" aaaaaaaaaaaaaaaa\n"
");",
Style);
+ verifyFormat(
+ "bool aaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
+ " const bool &aaaaaaaaa, const void *aaaaaaaaaa\n"
+ ") const {\n"
+ " return true;\n"
+ "}",
+ Style);
+ verifyFormat(
+ "bool aaaaaaaaaaaaaaaaaaaaaaaa(\n"
+ " const bool &aaaaaaaaaa, const void *aaaaaaaaaa\n"
+ ") const;",
+ Style);
+ verifyFormat(
+ "void aaaaaaaaa(\n"
+ " int aaaaaa, int bbbbbb, int cccccc, int dddddddddd\n"
+ ") const noexcept -> std::vector<of_very_long_type>;",
+ Style);
+ verifyFormat(
+ "aaaaaaaaaaaaaaaaaaa(\n"
+ " \"a aaaaaaa aaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaa aaaaaaaaaaaaa\"\n"
+ ");",
+ Style);
}
TEST_F(FormatTest, ParenthesesAndOperandAlignment) {
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| @@ -803,6 +803,60 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun, | |||
| return !Tok.Previous->isOneOf(TT_CastRParen, tok::kw_for, tok::kw_while, | |||
| tok::kw_switch); | |||
| }; | |||
| // Detecting functions is brittle. It would be better if we could annotate | |||
| // the LParen type of functions/calls. | |||
There was a problem hiding this comment.
I took a look, but didn't see a straightforward way to add support inside of parseParens
There was a problem hiding this comment.
I've done it in #97938. Can you use it after it's merged?
There was a problem hiding this comment.
I've done it in #97938. Can you use it after it's merged?
Yes, that works to replace the IsFunctionDeclParen, very nice thanks. I still need the hacky way of detection FunctionCallLParen.
owenca
reviewed
Jun 2, 2024
owenca
reviewed
Jun 2, 2024
owenca
reviewed
Jun 2, 2024
HazardyKnusperkeks
approved these changes
Jun 26, 2024
|
Rebased to be able to use #97938 |
|
I have another PR lined up to submit but it depends on this one. It would be great if we can get some forward progress @owenca |
|
You can annotate the lambda l_paren: |
owenca
reviewed
Jul 22, 2024
|
Please also add the following: |
owenca
reviewed
Jul 22, 2024
owenca
reviewed
Jul 22, 2024
Fixes llvm#55731 Fixes llvm#73584 The reported formatting problems were related to ignoring deep nesting of "simple" functions (causing llvm#54808) and to allowing the trailing annotation to become separated from the closing parens, which allowed a break to occur between the closing parens and the trailing annotation. The fix for the nesting of "simple" functions is to detect them more carefully. "Simple" was defined in a comment as being a single non-expression argument. I tried to stay as close to the original intent of the implementation while fixing the various bad formatting reports. In the process of fixing these bugs, some latent bugs were discovered related to how JavaScript Template Strings are handled. Those are also fixed here.
Seems it's not fixed yet: You can correct the commit message and fix the issue in another patch. |
owenca
approved these changes
Jul 24, 2024
Oh, yeah that makes sense. The function lparen parsing would not pick up braced initializer. |
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
Summary: Fixes #55731 The reported formatting problems were related to ignoring deep nesting of "simple" functions (causing #54808) and to allowing the trailing annotation to become separated from the closing parens, which allowed a break to occur between the closing parens and the trailing annotation. The fix for the nesting of "simple" functions is to detect them more carefully. "Simple" was defined in a comment as being a single non-expression argument. I tried to stay as close to the original intent of the implementation while fixing the various bad formatting reports. In the process of fixing these bugs, some latent bugs were discovered related to how JavaScript Template Strings are handled. Those are also fixed here. --------- Co-authored-by: Owen Pan <[email protected]> Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250557
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #55731
The reported formatting problems were related to ignoring deep nesting of "simple" functions (causing #54808) and to allowing the trailing annotation to become separated from the closing parens, which allowed a break to occur between the closing parens and the trailing annotation. The fix for the nesting of "simple" functions is to detect them more carefully. "Simple" was defined in a comment as being a single non-expression argument. I tried to stay as close to the original intent of the implementation while fixing the various bad formatting reports.
In the process of fixing these bugs, some latent bugs were discovered related to how JavaScript Template Strings are handled. Those are also fixed here.