[clang] [clang-format]: Fix formatting of if statements with BlockIndent (PR #77699)
Gedare Bloom via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 10 15:01:19 PST 2024
https://github.com/gedare created https://github.com/llvm/llvm-project/pull/77699
A bug with BlockIndent prevents line breaks within if (and else if) clauses.
While fixing this bug, it appears that AlignAfterOpenBracket is not designed to work with loop and if statements, but AlwaysBreak works on if clauses. The documentation and tests are not clear on whether or not this behavior is intended.
This PR preserves the `AlwaysBreak` behavior on `if` clauses without supporting `BlockIndent` on `if` clauses to avoid regressions while fixing the bug.
It may be reasonable to create an explicit option for alignment of if (and loop) clauses intentionally for both `AlwaysBreak` and `BlockIndent`
Fixes #54663.
Migrated from Differential Revision: https://reviews.llvm.org/D154755
See more discussion there. Addressed last open comment from the rev about refactoring the complex conditional logic involved with the `AlignAfterOpenBracket` line break behavior.
>From ff055d9c064d1fb359d59eeb47cb5f8c6c422bec Mon Sep 17 00:00:00 2001
From: Gedare Bloom <gedare at rtems.org>
Date: Fri, 7 Jul 2023 17:28:47 -0600
Subject: [PATCH 1/2] Fix formatting of if statements with BlockIndent
A bug with BlockIndent prevents line breaks within if (and else if) clauses.
While fixing this bug, it appears that AlignAfterOpenBracket is not designed
to work with loop and if statements, but AlwaysBreak works on if clauses.
The documentation and tests are not clear on whether or not this is intended.
This patch preserves the AlwaysBreak behavior and supports BlockIndent on if
clauses while fixing the bug.
It may be reasonable to go the other way and create an explicit option for
alignment of if (and loop) clauses intentionally.
Fixes #54663.
Differential Revision: https://reviews.llvm.org/D154755
---
clang/lib/Format/ContinuationIndenter.cpp | 6 ++--
clang/unittests/Format/FormatTest.cpp | 40 ++++++++++++++++++++---
2 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index 102504182c4505..8b288c62473db9 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -775,8 +775,10 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
Style.Cpp11BracedListStyle)) &&
State.Column > getNewLineColumn(State) &&
(!Previous.Previous ||
- !Previous.Previous->isOneOf(TT_CastRParen, tok::kw_for, tok::kw_while,
- tok::kw_switch)) &&
+ !(Previous.Previous->isOneOf(TT_CastRParen, tok::kw_for, tok::kw_while,
+ tok::kw_switch) ||
+ (Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent &&
+ Previous.Previous->isIf()))) &&
// Don't do this for simple (no expressions) one-argument function calls
// as that feels like needlessly wasting whitespace, e.g.:
//
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 25ef5c680af862..8d55e62e2558d3 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -25889,8 +25889,8 @@ TEST_F(FormatTest, AlignAfterOpenBracketBlockIndentIfStatement) {
"}",
Style);
- verifyFormat("if (quitelongarg !=\n"
- " (alsolongarg - 1)) { // ABC is a very longgggggggggggg "
+ verifyFormat("if (quiteLongArg !=\n"
+ " (alsoLongArg - 1)) { // ABC is a very longgggggggggggg "
"comment\n"
" return;\n"
"}",
@@ -25903,12 +25903,44 @@ TEST_F(FormatTest, AlignAfterOpenBracketBlockIndentIfStatement) {
"}",
Style);
- verifyFormat("if (quitelongarg !=\n"
- " (alsolongarg - 1)) { // ABC is a very longgggggggggggg "
+ verifyFormat("if (quiteLongArg !=\n"
+ " (alsoLongArg - 1)) { // ABC is a very longgggggggggggg "
"comment\n"
" return;\n"
"}",
Style);
+
+ verifyFormat("void foo() {\n"
+ " if (camelCaseName < alsoLongName ||\n"
+ " anotherEvenLongerName <=\n"
+ " thisReallyReallyReallyReallyReallyReallyLongerName ||"
+ "\n"
+ " otherName < thisLastName) {\n"
+ " return;\n"
+ " } else if (quiteLongName < alsoLongName ||\n"
+ " anotherEvenLongerName <=\n"
+ " thisReallyReallyReallyReallyReallyReallyLonger"
+ "Name ||\n"
+ " otherName < thisLastName) {\n"
+ " return;\n"
+ " }\n"
+ "}",
+ Style);
+
+ Style.ContinuationIndentWidth = 2;
+ verifyFormat("void foo() {\n"
+ " if (ThisIsRatherALongIfClause && thatIExpectToBeBroken ||\n"
+ " ontoMultipleLines && whenFormattedCorrectly) {\n"
+ " if (false) {\n"
+ " return;\n"
+ " } else if (thisIsRatherALongIfClause && "
+ "thatIExpectToBeBroken ||\n"
+ " ontoMultipleLines && whenFormattedCorrectly) {\n"
+ " return;\n"
+ " }\n"
+ " }\n"
+ "}",
+ Style);
}
TEST_F(FormatTest, AlignAfterOpenBracketBlockIndentForStatement) {
>From 55cfce78dbfc87af455e6bf822b8f0a6a5f9f3d6 Mon Sep 17 00:00:00 2001
From: Gedare Bloom <gedare at rtems.org>
Date: Wed, 10 Jan 2024 14:44:41 -0700
Subject: [PATCH 2/2] Refactor complex conditional logic as a lambda
---
clang/lib/Format/ContinuationIndenter.cpp | 25 +++++++++++++++--------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index 8b288c62473db9..29426c225fec9e 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -768,17 +768,24 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
// parenthesis by disallowing any further line breaks if there is no line
// break after the opening parenthesis. Don't break if it doesn't conserve
// columns.
+ auto isOpeningBracket = [&](const FormatToken &Tok) {
+ auto isStartOfBracedList = [&](const FormatToken &Tok) {
+ return Tok.is(tok::l_brace) && Tok.isNot(BK_Block) &&
+ Style.Cpp11BracedListStyle;
+ };
+ if (Tok.isOneOf(tok::l_paren, TT_TemplateOpener, tok::l_square) ||
+ isStartOfBracedList(Tok)) {
+ if (!Tok.Previous)
+ return true;
+ if (Tok.Previous->isIf())
+ return Style.AlignAfterOpenBracket == FormatStyle::BAS_AlwaysBreak;
+ return !Tok.Previous->isOneOf(TT_CastRParen, tok::kw_for, tok::kw_while,
+ tok::kw_switch);
+ }
+ };
if ((Style.AlignAfterOpenBracket == FormatStyle::BAS_AlwaysBreak ||
Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent) &&
- (Previous.isOneOf(tok::l_paren, TT_TemplateOpener, tok::l_square) ||
- (Previous.is(tok::l_brace) && Previous.isNot(BK_Block) &&
- Style.Cpp11BracedListStyle)) &&
- State.Column > getNewLineColumn(State) &&
- (!Previous.Previous ||
- !(Previous.Previous->isOneOf(TT_CastRParen, tok::kw_for, tok::kw_while,
- tok::kw_switch) ||
- (Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent &&
- Previous.Previous->isIf()))) &&
+ isOpeningBracket(Previous) && State.Column > getNewLineColumn(State) &&
// Don't do this for simple (no expressions) one-argument function calls
// as that feels like needlessly wasting whitespace, e.g.:
//
More information about the cfe-commits
mailing list