[clang-format] Fix the indent of the ternary operator when AlignOperands and BreakBeforeTernaryOperators is specified. #100860
Conversation
This file contains hidden or 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
…nds and BreakBeforeTernaryOperators is specified.
Member
|
@llvm/pr-subscribers-clang-format Author: Hirofumi Nakamura (hnakamura5) ChangesTrying to fix #98559 . This PR modifies the ad-hoc tweaks introduced by https://reviews.llvm.org/D50078 . ↓ This PR makes, For the case So I did not touch the case at first. It may be a point of discussion. Full diff: https://github.com/llvm/llvm-project/pull/100860.diff 3 Files Affected:
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index b07360425ca6e..c01fe6940bf22 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -1347,8 +1347,14 @@ unsigned ContinuationIndenter::getNewLineColumn(const LineState &State) {
// * always un-indent by the operator when
// BreakBeforeTernaryOperators=true
unsigned Indent = CurrentState.Indent;
- if (Style.AlignOperands != FormatStyle::OAS_DontAlign)
+ if (Style.AlignOperands != FormatStyle::OAS_DontAlign &&
+ !Style.BreakBeforeTernaryOperators) {
Indent -= Style.ContinuationIndentWidth;
+ }
+ if (Style.BreakBeforeTernaryOperators &&
+ CurrentState.IsChainedConditional) {
+ Indent -= 2;
+ }
if (Style.BreakBeforeTernaryOperators && CurrentState.UnindentOperator)
Indent -= 2;
return Indent;
@@ -1773,7 +1779,8 @@ void ContinuationIndenter::moveStatePastFakeLParens(LineState &State,
!CurrentState.IsWrappedConditional) {
NewParenState.IsChainedConditional = true;
NewParenState.UnindentOperator = State.Stack.back().UnindentOperator;
- } else if (PrecedenceLevel == prec::Conditional ||
+ } else if ((PrecedenceLevel == prec::Conditional &&
+ !Style.BreakBeforeTernaryOperators) ||
(!SkipFirstExtraIndent && PrecedenceLevel > prec::Assignment &&
!Current.isTrailingComment())) {
NewParenState.Indent += Style.ContinuationIndentWidth;
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index d01ce137b8fcb..bdd657db06712 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -7332,12 +7332,11 @@ TEST_F(FormatTest, ExpressionIndentationBreakingBeforeOperators) {
"};",
Style);
verifyFormat("return abc\n"
- " ? foo(\n"
- " a,\n"
- " b,\n"
- " bar(\n"
- " abc))\n"
- " : g(abc);",
+ " ? foo(\n"
+ " a,\n"
+ " b,\n"
+ " bar(abc))\n"
+ " : g(abc);",
Style);
}
@@ -9410,20 +9409,20 @@ TEST_F(FormatTest, BreaksConditionalExpressions) {
" : aaaaaaaaaaaaaaaaaaaaaaaaaaa;");
verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaa =\n"
" aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
- " ? aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
- " : aaaaaaaaaaaaaaaa;");
+ " ? aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+ " : aaaaaaaaaaaaaaaa;");
verifyFormat(
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa == aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
" ? aaaaaaaaaaaaaaa\n"
" : aaaaaaaaaaaaaaa;");
verifyFormat("f(aaaaaaaaaaaaaaaa == // force break\n"
- " aaaaaaaaa\n"
- " ? b\n"
- " : c);");
+ " aaaaaaaaa\n"
+ " ? b\n"
+ " : c);");
verifyFormat("return aaaa == bbbb\n"
- " // comment\n"
- " ? aaaa\n"
- " : bbbb;");
+ " // comment\n"
+ " ? aaaa\n"
+ " : bbbb;");
verifyFormat("unsigned Indent =\n"
" format(TheLine.First,\n"
" IndentForLevel[TheLine.Level] >= 0\n"
@@ -9432,21 +9431,20 @@ TEST_F(FormatTest, BreaksConditionalExpressions) {
" TheLine.InPPDirective, PreviousEndOfLineColumn);",
getLLVMStyleWithColumns(60));
verifyFormat("bool aaaaaa = aaaaaaaaaaaaa //\n"
- " ? aaaaaaaaaaaaaaa\n"
- " : bbbbbbbbbbbbbbb //\n"
- " ? ccccccccccccccc\n"
- " : ddddddddddddddd;");
+ " ? aaaaaaaaaaaaaaa\n"
+ " : bbbbbbbbbbbbbbb //\n"
+ " ? ccccccccccccccc\n"
+ " : ddddddddddddddd;");
verifyFormat("bool aaaaaa = aaaaaaaaaaaaa //\n"
- " ? aaaaaaaaaaaaaaa\n"
- " : (bbbbbbbbbbbbbbb //\n"
- " ? ccccccccccccccc\n"
- " : ddddddddddddddd);");
- verifyFormat(
- "int aaaaaaaaaaaaaaaaaaaaaaaaaaa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
- " ? aaaaaaaaaaaaaaaaaaaaaaaaa +\n"
- " aaaaaaaaaaaaaaaaaaaaa +\n"
- " aaaaaaaaaaaaaaaaaaaaa\n"
- " : aaaaaaaaaa;");
+ " ? aaaaaaaaaaaaaaa\n"
+ " : (bbbbbbbbbbbbbbb //\n"
+ " ? ccccccccccccccc\n"
+ " : ddddddddddddddd);");
+ verifyFormat("int aaaaaaaaaaaaaaaaaaaaaaaaaaa =\n"
+ " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+ " ? aaaaaaaaaaaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaaaaaaa + "
+ "aaaaaaaaaaaaaaaaaaaaa\n"
+ " : aaaaaaaaaa;");
verifyFormat(
"aaaaaa = aaaaaaaaaaaa ? aaaaaaaaaa ? aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
" : aaaaaaaaaaaaaaaaaaaaaa\n"
@@ -9480,23 +9478,23 @@ TEST_F(FormatTest, BreaksConditionalExpressions) {
// Assignments in conditional expressions. Apparently not uncommon :-(.
verifyFormat("return a != b\n"
- " // comment\n"
- " ? a = b\n"
- " : a = b;");
+ " // comment\n"
+ " ? a = b\n"
+ " : a = b;");
verifyFormat("return a != b\n"
- " // comment\n"
- " ? a = a != b\n"
- " // comment\n"
- " ? a = b\n"
- " : a\n"
- " : a;");
+ " // comment\n"
+ " ? a = a != b\n"
+ " // comment\n"
+ " ? a = b\n"
+ " : a\n"
+ " : a;");
verifyFormat("return a != b\n"
- " // comment\n"
- " ? a\n"
- " : a = a != b\n"
- " // comment\n"
- " ? a = b\n"
- " : a;");
+ " // comment\n"
+ " ? a\n"
+ " : a = a != b\n"
+ " // comment\n"
+ " ? a = b\n"
+ " : a;");
// Chained conditionals
FormatStyle Style = getLLVMStyleWithColumns(70);
@@ -9531,26 +9529,26 @@ TEST_F(FormatTest, BreaksConditionalExpressions) {
" : (aaa ? bbb : ccc);",
Style);
verifyFormat(
- "return aaaaaaaaaaaaaaaa ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
- " : cccccccccccccccccc)\n"
+ "return aaaaaaaaaaaaaaaa\n"
+ " ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb : cccccccccccccccccc)\n"
" : bbbbbbbbbbbbbb ? 2222222222222222\n"
" : 3333333333333333;",
Style);
verifyFormat(
- "return aaaaaaaaa ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
- " : cccccccccccccccccc)\n"
+ "return aaaaaaaaa\n"
+ " ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb : cccccccccccccccccc)\n"
" : bbbbbbbbbbbbbb ? 2222222222222222\n"
" : 3333333333333333;",
Style);
verifyFormat(
- "return aaaaaaaaa ? a = (aaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
- " : dddddddddddddddddd)\n"
+ "return aaaaaaaaa\n"
+ " ? a = (aaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb : dddddddddddddddddd)\n"
" : bbbbbbbbbbbbbb ? 2222222222222222\n"
" : 3333333333333333;",
Style);
verifyFormat(
- "return aaaaaaaaa ? a + (aaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
- " : dddddddddddddddddd)\n"
+ "return aaaaaaaaa\n"
+ " ? a + (aaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb : dddddddddddddddddd)\n"
" : bbbbbbbbbbbbbb ? 2222222222222222\n"
" : 3333333333333333;",
Style);
@@ -9588,27 +9586,27 @@ TEST_F(FormatTest, BreaksConditionalExpressions) {
" : 3333333333333333;",
Style);
verifyFormat(
- "return aaaaaaaaaaaaaaaa ? aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
- " : cccccccccccccccccc\n"
+ "return aaaaaaaaaaaaaaaa\n"
+ " ? aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb : cccccccccccccccccc\n"
" : bbbbbbbbbbbbbb ? 2222222222222222\n"
" : 3333333333333333;",
Style);
verifyFormat(
- "return aaaaaaaaaaaaaaaa ? aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
- " : cccccccccccccccc ? dddddddddddddddddd\n"
- " : eeeeeeeeeeeeeeeeee\n"
+ "return aaaaaaaaaaaaaaaa ? aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+ " : cccccccccccccccc ? dddddddddddddddddd\n"
+ " : eeeeeeeeeeeeeeeeee\n"
" : bbbbbbbbbbbbbb ? 2222222222222222\n"
" : 3333333333333333;",
Style);
verifyFormat("return aaaaaaaaaaaaaaaaaaaaa\n"
- " ? (aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
- " : cccccccccccccccccc ? dddddddddddddddddd\n"
- " : eeeeeeeeeeeeeeeeee)\n"
+ " ? (aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+ " : cccccccccccccccccc ? dddddddddddddddddd\n"
+ " : eeeeeeeeeeeeeeeeee)\n"
" : bbbbbbbbbbbbbbbbbbb ? 2222222222222222\n"
" : 3333333333333333;",
Style);
verifyFormat("return aaaaaaaaaaaaaaaaaaaaaaaaa\n"
- " ? aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+ " ? aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
" : cccccccccccccccc ? dddddddddddddddddd\n"
" : eeeeeeeeeeeeeeeeee\n"
" : bbbbbbbbbbbbbbbbbbbbbbb ? 2222222222222222\n"
@@ -26490,11 +26488,11 @@ TEST_F(FormatTest, MultilineLambdaInConditional) {
Style = getLLVMStyleWithColumns(60);
verifyFormat("auto aLengthyIdentifier = oneExpressionSoThatWeBreak\n"
- " ? []() {\n"
- " ;\n"
- " return 5;\n"
- " }()\n"
- " : 2;",
+ " ? []() {\n"
+ " ;\n"
+ " return 5;\n"
+ " }()\n"
+ " : 2;",
Style);
verifyFormat("auto aLengthyIdentifier =\n"
" oneExpressionSoThatWeBreak ? 2 : []() {\n"
@@ -26513,11 +26511,11 @@ TEST_F(FormatTest, MultilineLambdaInConditional) {
Style);
verifyFormat("auto aLengthyIdentifier =\n"
" oneExpressionSoThatWeBreak\n"
- " ? 2\n"
- " : []() {\n"
- " ;\n"
- " return 5;\n"
- " };",
+ " ? 2\n"
+ " : []() {\n"
+ " ;\n"
+ " return 5;\n"
+ " };",
Style);
}
diff --git a/clang/unittests/Format/FormatTestRawStrings.cpp b/clang/unittests/Format/FormatTestRawStrings.cpp
index 0615fb1fad4c5..c21172c6ac7ad 100644
--- a/clang/unittests/Format/FormatTestRawStrings.cpp
+++ b/clang/unittests/Format/FormatTestRawStrings.cpp
@@ -642,8 +642,8 @@ auto S=R"pb(item_1:1)pb"+R"pb(item_2:2)pb"+R"pb(item_3:3)pb";
expect_eq(R"test(
auto S = (count < 3)
- ? R"pb(item_1: 1)pb"
- : R"pb(item_2: 2)pb";
+ ? R"pb(item_1: 1)pb"
+ : R"pb(item_2: 2)pb";
)test",
format(R"test(
auto S=(count<3)?R"pb(item_1:1)pb":R"pb(item_2:2)pb";
@@ -651,10 +651,9 @@ auto S=(count<3)?R"pb(item_1:1)pb":R"pb(item_2:2)pb";
getRawStringPbStyleWithColumns(40)));
expect_eq(R"test(
-auto S =
- (count < 3)
- ? R"pb(item_1: 1, item_2: 2)pb"
- : R"pb(item_3: 3)pb";
+auto S = (count < 3)
+ ? R"pb(item_1: 1, item_2: 2)pb"
+ : R"pb(item_3: 3)pb";
)test",
format(R"test(
auto S=(count<3)?R"pb(item_1:1,item_2:2)pb":R"pb(item_3:3)pb";
@@ -664,8 +663,8 @@ auto S=(count<3)?R"pb(item_1:1,item_2:2)pb":R"pb(item_3:3)pb";
expect_eq(R"test(
auto S =
(count < 3)
- ? R"pb(item_1: 1)pb"
- : R"pb(item_2: 2, item_3: 3)pb";
+ ? R"pb(item_1: 1)pb"
+ : R"pb(item_2: 2, item_3: 3)pb";
)test",
format(R"test(
auto S=(count<3)?R"pb(item_1:1)pb":R"pb(item_2:2,item_3:3)pb";
|
HazardyKnusperkeks
requested changes
Jul 28, 2024
Contributor
HazardyKnusperkeks
left a comment
HazardyKnusperkeks
left a comment
There was a problem hiding this comment.
You are changing a lot of test cases, that means you will break a lot of formattings out there, especially since this is the default behavior of clang-format. And someone did intend it that way.
If the change is desired, the BreakBeforeTernaryOperators option should be expanded to an enum or AlignOperands extended.
Contributor
Author
I agree. The current style has been default in this many years. Enough time to be got familiar with. https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignoperands The issue #98559 can also be regarded as the inaccuracy of the manual. That says
Only changing this part may be better. If the precise alignment is desired, adding a new |
Contributor
|
I feel I remember this be explicitly added this way, I think this is functioning as designed if you want something different you'll need a new option. Changing tests is a dead give away. |
Member
|
In Nix (github.com/nixos/nix) I am getting formatting like auto variant = scheme == "auto" ? (StoreReference::Variant{Specified::Auto{}})
: (StoreReference::Variant{Specified::Specified{
.scheme = getString,
.authority = getString(valueAt(obj, "authority")),
}});and I am not really sure why, but I gather it might have something to do with this. IMO a key rule of formatting to me is that it should not depend on the the exact lengths of identifiers and literals. Making the auto variant = scheme == "auto"
? (StoreReference::Variant{Specified::Auto{}})
: (StoreReference::Variant{Specified::Specified{
.scheme = getString,
.authority = getString(valueAt(obj, "authority")),
}});doesn't depend on those things, and so is a a valid choice. I would hope there would at least be some combination of settings to enforce this invariant. |
mydeveloperday
requested changes
Mar 21, 2025
Contributor
mydeveloperday
left a comment
mydeveloperday
left a comment
There was a problem hiding this comment.
This change is subjective not objective. Other people will have other views as such this can't come in like this or we'll change 1000s lines of code without people having control to keep this current style.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Trying to fix #98559 .
This PR modifies the ad-hoc tweaks introduced by https://reviews.llvm.org/D50078 .
The change is limited to the case
AlignOperandsandBreakBeforeTernaryOperatorsare specified (default in LLVM style).↓ This PR makes,
For the case
BreakBeforeTernaryOperatorsis not specified, I'm not sure such style is desirable.So I did not touch the case at first. It may be a point of discussion.