[clang-format] Fix the continuation indenter

[owenpan]

Summary

When Style.BreakBeforeBinaryOperators is set to FormatStyle::BOS_NonAssignment,
arguments following after a line break should be indented further, which they are
currently not. This attempts to fix that.

[HazardyKnusperkeks]

clang/lib/Format/ContinuationIndenter.cpp

813–816

Add braces (and drop the plus).

clang/unittests/Format/FormatTest.cpp

6599–6601

Do you need this for the observed effect? From the description I don't see that.

6603

Is the behavior different when the code is already formatted like you want it? (It shouldn't.) Then you should use verifyFormat().

[MyDeveloperDay]

Can we log a GitHub issue I can’t see what you are trying to fix

[MyDeveloperDay]

Is it seeing the ‘ :’ as a binary operator? And not an inheritance colon?

[MyDeveloperDay]

Needs a better title to the review please to explain what it’s doing

[hel-ableton]

Sorry about the plus sign! I'm fairly unfamiliar with your process, so something between making a diff and making a patch must have gone wrong. About the brackets, I was wondering why there weren't any, but didn't see the need to add them given the change. In any case, now there are brackets.

[hel-ableton]

In D136154#3867328, @MyDeveloperDay wrote:

Can we log a GitHub issue I can’t see what you are trying to fix

I'm sorry if this is unclear. The background to this is that our repository is currently formatted using clang-format 6.0.0, and we regard this as a bug that must have been introduced somewhere between 7.1.0 and 10.0.0, which is resulting in unnecessary reformatting of a lot of code, and is what kept us from upgrading clang-format for quite a while now. Other circumstances now force us to upgrade, though, and we would like to avoid introducing these changes, not only for diff noise, but also because we don't think the indentation makes sense that way.

A GitHub issue would be fine by me, also I could use some help with how to effectively work with the llvm-project repo, as currently I'm suffering a bit from huge turnaround times, like having to recompile lots and lots of code if I want to compare the result of formatting with or without the proposed fix.

[hel-ableton]

In D136154#3867328, @MyDeveloperDay wrote:

Can we log a GitHub issue I can’t see what you are trying to fix

Without the fix, the arguments to the Base class would be on the same level as the Base class itself. But this also effects other code. I'll attach too screenshots, the diffs are going from with the fix to without the fix.

[hel-ableton]

must have been introduced somewhere between 7.1.0 and 10.0.0

In fact, someone in our team thought that this must have been introduced exactly with this commit:

https://github.com/llvm/llvm-project/commit/4636debc271f09f730697ab6873137a657c828f9

[hel-ableton]

Adding two inline comments.

clang/unittests/Format/FormatTest.cpp

6599–6601

I think it's necessary for the test to work, otherwise the value would fall back to 4, I think? Would you prefer the test to be re-written with the default value, instead?

6603

Not sure I understand. Would you like the test to be re-written so that the code is already formatted like the result of the formatting?

[HazardyKnusperkeks]

In D136154#3867461, @hel-ableton wrote:

must have been introduced somewhere between 7.1.0 and 10.0.0

In fact, someone in our team thought that this must have been introduced exactly with this commit:

https://github.com/llvm/llvm-project/commit/4636debc271f09f730697ab6873137a657c828f9

You need to check that the operator is not an assignment, maybe add more test cases with the 3 options to show how it should behave.

[MyDeveloperDay]

You've dropped the test on your most recent updated

clang/unittests/Format/FormatTest.cpp

6603–6622

The starting state of the code should not be important, verifyFormat will "messUp" the code spacing to test this out, your are verifiying that it maintains what you want.

[MyDeveloperDay]

I get why this fix works for you, but I don't agree that the fix is the correct one..

for example you don't have any BinaryOperators in that code, lets just say in my style I had

BreakBeforeBinaryOperators: All

Then your fix wouldn't cover me and I'd get exactly the same issue, (but why should i)

struct Foo {
  Foo(
    int firstArgWithLongName,
    int secondArgWithLongName,
    int thirdArgWithLongName,
    int fourthArgWithLongName)
      : Base(
        firstArgWithLongName,
        secondArgWithLongName,
        thirdArgWithLongName,
        fourthArgWithLongName) {}

The problem as I see it was that the original bug, highly constrained the cases where "CurrentState.LastSpace = State.Column;" to one particular style (which if it happens to be your style great but not if its not.

[MyDeveloperDay]

Its probably worth looking at how to fix this more in the context of the original bug, Its something I don't like is where we pile in all these expressions, without any comments about what cases we are handling here..
I'd rather handle each one at a time.. as I don't see how TT_BinaryOperator and TT_CtorInitializerColon are likely to be related.

else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr,
                               TT_CtorInitializerColon)) &&
             ((Previous.getPrecedence() != prec::Assignment &&
               (Previous.isNot(tok::lessless) || Previous.OperatorIndex != 0 ||
                Previous.NextOperator)) ||
              Current.StartsBinaryExpression)) {
    // Indent relative to the RHS of the expression unless this is a simple
    // assignment without binary expression on the RHS. Also indent relative to
    // unary operators and the colons of constructor initializers.
    if (Style.BreakBeforeBinaryOperators == FormatStyle::BOS_None ||
+       Style.BreakBeforeBinaryOperators == FormatStyle::BOS_NonAssignment)
      CurrentState.LastSpace = State.Column;

[MyDeveloperDay]

Doesn't something like this achieve the same

     CurrentState.Indent = State.Column;
     CurrentState.LastSpace = State.Column;
-  } else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr,
-                               TT_CtorInitializerColon)) &&
+  } else if (Previous.is(TT_CtorInitializerColon)) {
+    CurrentState.LastSpace = State.Column;
+  } else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr)) &&
              ((Previous.getPrecedence() != prec::Assignment &&
                (Previous.isNot(tok::lessless) || Previous.OperatorIndex != 0 ||

Regardless of the choice of BreakBeforeBinaryOperators?

BTW I checked such a change passes all the other unit tests. (Beyonce rule!!)

[MyDeveloperDay]

I think in hindsight D50699: [clang-format] fix PR38525 - Extraneous continuation indent spaces with BreakBeforeBinaryOperators set to All was overly aggressive again the TT_CtorInitializerColon case

[hel-ableton]

In D136154#3867935, @MyDeveloperDay wrote:

You've dropped the test on your most recent updated

Oh, that's why it appeared from the diff. Apologies again, I'm really unfamiliar with your process. I guess it puzzles me why it's described in https://llvm.org/docs/Phabricator.html#phabricator-request-review-web that I should make any commits at all, when it's just diffs that I'm supposed to submit. Anyway, I'll try to bring it back.

[hel-ableton]

This should bring back the formerly introduced unit test.

[hel-ableton]

The problem as I see it was that the original bug, highly constrained the cases where "CurrentState.LastSpace = State.Column;" to one particular style (which if it happens to be your style great but not if its not.

You mean the original bugfix was already unnecessarily constrained, and now my proposed fix is only opening it up for one my case? That may be so. In any case this might really not be the ideal fix, and I'm open to any other, better way of fixing it.

[hel-ableton]

Using verifyFormat() now instead of EXPECT_EQ()

[hel-ableton]

In D136154#3867968, @MyDeveloperDay wrote:

Doesn't something like this achieve the same

     CurrentState.Indent = State.Column;
     CurrentState.LastSpace = State.Column;
-  } else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr,
-                               TT_CtorInitializerColon)) &&
+  } else if (Previous.is(TT_CtorInitializerColon)) {
+    CurrentState.LastSpace = State.Column;
+  } else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr)) &&
              ((Previous.getPrecedence() != prec::Assignment &&
                (Previous.isNot(tok::lessless) || Previous.OperatorIndex != 0 ||

Regardless of the choice of BreakBeforeBinaryOperators?

BTW I checked such a change passes all the other unit tests. (Beyonce rule!!)

If you think this would be the better fix, AFAICS it does what we need. What's the Beyonce rule, by the way?

[MyDeveloperDay]

Beyonce rule

"If they liked it they should have put a test on it"

My view is that any change we make is that passes all the tests is valid! (sort of!), i.e. if someone else relied on a particular set of functionality then they should have put a test in it.

This is why I don't like changing tests. The fact that the the original change from 2018 passes with this fix means we are good to go.

[MyDeveloperDay]

In D136154#3868151, @hel-ableton wrote:

If you think this would be the better fix, AFAICS it does what we need. What's the Beyonce rule, by the way?

I do, but I'd like to hear @owenpan and @HazardyKnusperkeks view.. (each of us is better in different part of clang-format, and I value their opinion!)

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

6634

missing newline

[hel-ableton]

Fixed missing newline.

[hel-ableton]

I do, but I'd like to hear @owenpan and @HazardyKnusperkeks view.. (each of us is better in different part of clang-format, and I value their opinion!)

Fair enough.

[MyDeveloperDay]

clang/lib/Format/ContinuationIndenter.cpp

814

ok so now you don't actually have a test for this, so I don't know if this is needed any more.

clang/unittests/Format/FormatTest.cpp

6602

I don't think you need this line

[MyDeveloperDay]

Looking at the documentation, I think BreakBeforeBinaryOperators is unrelated to your test case. I would remove any references of it from your fix.

[MyDeveloperDay]

Going back to the original bug that caused this.. if you leave in your BOS_NonAssignment I think you'll reintroduce the original bug.. which possibly should have been called

"incorrectly adds extra continuation indent spaces with BreakBeforeBinaryOperators set to All *(and NonAssignement)*"

i.e.

bool BreakBeforeBinaryOperators(
        bool someVeryVeryLongConditionThatBarelyFitsOnALine,
        bool someOtherLongishConditionPart1,
        bool someOtherEvenLongerNestedConditionPart2) {
        return someVeryVeryLongConditionThatBarelyFitsOnALine
                && (someOtherLongishConditionPart1
                        || someOtherEvenLongerNestedConditionPart2);
}

will bcome

bool BreakBeforeBinaryOperators(
        bool someVeryVeryLongConditionThatBarelyFitsOnALine,
        bool someOtherLongishConditionPart1,
        bool someOtherEvenLongerNestedConditionPart2) {
        return someVeryVeryLongConditionThatBarelyFitsOnALine
                && (someOtherLongishConditionPart1
                           || someOtherEvenLongerNestedConditionPart2);
                     ^^^^^^^^
}

[HazardyKnusperkeks]

In D136154#3868036, @hel-ableton wrote:

In D136154#3867935, @MyDeveloperDay wrote:

You've dropped the test on your most recent updated

Oh, that's why it appeared from the diff. Apologies again, I'm really unfamiliar with your process. I guess it puzzles me why it's described in https://llvm.org/docs/Phabricator.html#phabricator-request-review-web that I should make any commits at all, when it's just diffs that I'm supposed to submit. Anyway, I'll try to bring it back.

How you get your diff doesn't matter. I make the real commits (which I will commit, if approved), and then just do git diff -U9999999 HEAD^1 > file.

In D136154#3868070, @hel-ableton wrote:

The problem as I see it was that the original bug, highly constrained the cases where "CurrentState.LastSpace = State.Column;" to one particular style (which if it happens to be your style great but not if its not.

You mean the original bugfix was already unnecessarily constrained, and now my proposed fix is only opening it up for one my case? That may be so. In any case this might really not be the ideal fix, and I'm open to any other, better way of fixing it.

I too think one should split the conditions, as good as one can overview them. And add tests for each case. It seems you still have a running clang-format 6, then you are in the perfect place to regression test against that without any additional overhead.

Please mark all comments as done, if they are done.

clang/lib/Format/ContinuationIndenter.cpp

803

Before it was followed with an &&, I don't know which one was for this case, and which one for the other, or for both. This is a bit hard, but maybe this is just to few checks?

clang/unittests/Format/FormatTest.cpp

6599–6601

At least the indentation width shouldn't matter and changing that here can give the impression it does.

[hel-ableton]

I'm not sure I'm following where you're getting at. So far I'm getting the following:

• my proposed fix was not ideal, and only "accidentally" fixed our issue
• the fix including Previous.isOneOf(TT_BinaryOperator... is a better fix
• we should write a proper test case for that fix, as the one I submitted referred to the wrong fix
• the example you gave now (as a model to construct such a better test?) doesn't involve TT_CtorInitializerColon or any of the like, so...

I'm confused. :-)

...oh and one more process question: why is that I write answers to your comments and then they seem to be "drafts". I don't know how to submit them, and hope you get to see them anyway.

[MyDeveloperDay]

You can remove this line in my view...

Style.BreakBeforeBinaryOperators == FormatStyle::BOS_NonAssignment) {

[owenpan]

In D136154#3867328, @MyDeveloperDay wrote:

Can we log a GitHub issue I can’t see what you are trying to fix

@hel-ableton Can you log an issue for this and add a link to it in SUMMARY?

In D136154#3867441, @hel-ableton wrote:

I'll attach too screenshots, the diffs are going from with the fix to without the fix.

Can you include a minimal test case that reproduces the issue with the latest clang-format? Please also include your .clang-format file.

In D136154#3871389, @hel-ableton wrote:

...oh and one more process question: why is that I write answers to your comments and then they seem to be "drafts". I don't know how to submit them, and hope you get to see them anyway.

After you write a New Inline Comment, click the Save Draft button. Then click the Submit button near the bottom of the screen.

[hel-ableton]

After you write a New Inline Comment, click the Save Draft button. Then click the Submit button near the bottom of the screen.

Makes sense, only I see no "Save Draft" button...

clang/lib/Format/ContinuationIndenter.cpp

814

Yes, this test can probably be ditched once we find a more fitting one. Assuming that there is agreement on your side about the proposed other fix with the TT_... tokens.

[hel-ableton]

I filed an issue now in the llvm repo, it just describes how the formatting in our case changes, and what _might_ be done to keep it from doing so. I have consciously left out any proposed unit tests, because at this point I'm honestly too confused as to what would be the best approach there. I hope we can somewhat start from square 1 again with this. Sorry if I'm not submitting an ideal solution here, but I'm afraid I just know too little about how the code and the tests work exactly.

https://github.com/llvm/llvm-project/issues/58592

[owenpan]

clang/lib/Format/ContinuationIndenter.cpp

803–808

IMO we should make an NFC patch to clean this up before attempting to fix the bug: e.g. TT_ConditionalExpr and TT_CtorInitializerColon don't have prec::Assignment, aren't tok::lessless, etc.

[owenpan]

clang/lib/Format/ContinuationIndenter.cpp

803–804

Alternatively, we can fix the bug (except for assignment statements) by doing the above.

[owenpan]

In D136154#3882163, @hel-ableton wrote:

I hope we can somewhat start from square 1 again with this.

See https://reviews.llvm.org/D136154#3890747. It doesn't "fix" the last example in https://github.com/llvm/llvm-project/issues/58592, but it seems that the formatting of that example is caused by something else.

[owenpan]

@hel-ableton do you intend to continue working on this? If not, we can commandeer it and finish or abandon it.

[hel-ableton]

Hi Owen,
thanks for reaching out, no. In fact we realized that we don't have enough capacity to be working on it.

Best, Henrik

Henrik Lafrenz, Ableton AG
Schoenhauser Allee 6-7, 10119 Berlin, Germany
T: [phone]

Board: Gerhard Behles, Jan Bohl
Supervisory board: Uwe Struck (Chairman)
Register: Amtsgericht Berlin-Charlottenburg HRB 72838

---

Von: Owen Pan via Phabricator <[e-mail]>
Gesendet: Donnerstag, 14. September 2023 02:37
An: Henrik Lafrenz <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>
Cc: [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>
Betreff: [PATCH] D136154: [clang-format] Fix the continuation indenter

owenpan added a comment.
Herald added a reviewer: MyDeveloperDay.

@hel-ableton do you intend to continue working on this? If not, we can commandeer it and finish or abandon it.

CHANGES SINCE LAST ACTION

https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

clang/lib/Format/ContinuationIndenter.cpp

CurrentState.NestedBlockIndent = State.Column;
               (Previous.is(tok::colon) && Previous.is(TT_ObjCMethodExpr)))) {
     CurrentState.LastSpace = State.Column;
   } else if (Previous.is(TT_CtorInitializerColon) &&
              (!Current.isTrailingComment() || Current.NewlinesBefore > 0) &&
              Style.BreakConstructorInitializers ==
                  FormatStyle::BCIS_AfterColon) {
     CurrentState.Indent = State.Column;
     CurrentState.LastSpace = State.Column;
-  } else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr,
-                               TT_CtorInitializerColon)) &&
+    } else if (Previous.is(TT_CtorInitializerColon)) {
+      CurrentState.LastSpace = State.Column;
+    } else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr)) &&
              ((Previous.getPrecedence() != prec::Assignment &&
                (Previous.isNot(tok::lessless) || Previous.OperatorIndex != 0 ||
                 Previous.NextOperator)) ||
               Current.StartsBinaryExpression)) {
     // Indent relative to the RHS of the expression unless this is a simple
     // assignment without binary expression on the RHS. Also indent relative to
     // unary operators and the colons of constructor initializers.
-    if (Style.BreakBeforeBinaryOperators == FormatStyle::BOS_None)
+    if (Style.BreakBeforeBinaryOperators == FormatStyle::BOS_None ||
+        Style.BreakBeforeBinaryOperators == FormatStyle::BOS_NonAssignment) {
       CurrentState.LastSpace = State.Column;
+    }
   } else if (Previous.is(TT_InheritanceColon)) {
     CurrentState.Indent = State.Column;
     CurrentState.LastSpace = State.Column;
   } else if (Current.is(TT_CSharpGenericTypeConstraintColon)) {
     CurrentState.ColonPos = State.Column;
   } else if (Previous.opensScope()) {
     // If a function has a trailing call, indent all parameters from the
     // opening parenthesis. This avoids confusing indents like:

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ExpressionIndentationBreakingBeforeOperators) {
   Style.AlignOperands = FormatStyle::OAS_DontAlign;
   EXPECT_EQ("return someVeryVeryLongConditionThatBarelyFitsOnALine\n"
             "\t&& (someOtherLongishConditionPart1\n"
             "\t\t|| someOtherEvenLongerNestedConditionPart2);",
             format("return someVeryVeryLongConditionThatBarelyFitsOnALine && "
                    "(someOtherLongishConditionPart1 || "
                    "someOtherEvenLongerNestedConditionPart2);",
                    Style));
+
+  Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BinPackParameters = false;
+  Style.ContinuationIndentWidth = 2;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  verifyFormat(
+      "struct Derived {\n"
+      "  Derived(\n"
+      "    int firstArgWithLongName,\n"
+      "    int secondArgWithLongName,\n"
+      "    int thirdArgWithLongName,\n"
+      "    int fourthArgWithLongName)\n"
+      "      : Base(\n"
+      "          firstArgWithLongName,\n"
+      "          secondArgWithLongName,\n"
+      "          thirdArgWithLongName,\n"
+      "          fourthArgWithLongName) {}\n"
+      "};", Style);
 }
 
 TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
   Style.AlignOperands = FormatStyle::OAS_AlignAfterOperator;
 
   verifyFormat("bool value = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "                   + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "                   + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "              == aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "                         * bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\n"
                "                     + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\n"
                "          && aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "                     * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "                 > ccccccccccccccccccccccccccccccccccccccccc;",
                Style);
   verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "            * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "        + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "    == bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb) {\n}",
                Style);
   verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "        + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "              * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "    == bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb) {\n}",
                Style);