[PATCH] D136154: Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton created this revision.
hel-ableton added reviewers: JonasToth, MyDeveloperDay, owenpan.
hel-ableton added a project: clang-format.
Herald added a project: All.
hel-ableton requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136154

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6594,6 +6594,32 @@
"(someOtherLongishConditionPart1 || "
"someOtherEvenLongerNestedConditionPart2);",
Style));
+
+  Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BinPackParameters = false;
+  Style.ContinuationIndentWidth = 2;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  EXPECT_EQ(
+  "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"
+  "};",
+  format("struct Derived {"
+ "  Derived(int firstArgWithLongName, int secondArgWithLongName, "
+ "int thirdArgWithLongName, int fourthArgWithLongName)"
+ ": Base(firstArgWithLongName, secondArgWithLongName, "
+ "thirdArgWithLongName, fourthArgWithLongName) {}"
+ "};",
+ Style));
 }
 
 TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -809,7 +809,8 @@
 // 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;


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6594,6 +6594,32 @@
"(someOtherLongishConditionPart1 || "
"someOtherEvenLongerNestedConditionPart2);",
Style));
+
+  Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BinPackParameters = false;
+  Style.ContinuationIndentWidth = 2;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  EXPECT_EQ(
+  "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"
+  "};",
+  format("struct Derived {"
+ "  Derived(int firstArgWithLongName, int secondArgWithLongName, "
+ "int thirdArgWithLongName, int fourthArgWithLongName)"
+ ": Base(firstArgWithLongName, secondArgWithLongName, "
+ "thirdArgWithLongName, fourthArgWithLongName) {}"
+ "};",
+ Style));
 }
 
 TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -809,7 +809,8 @@
 // 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 == Forma

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton updated this revision to Diff 468793.
hel-ableton added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

Files:
  clang/lib/Format/ContinuationIndenter.cpp


Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -810,8 +810,9 @@
 // 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)
+Style.BreakBeforeBinaryOperators == FormatStyle::BOS_NonAssignment) {
   CurrentState.LastSpace = State.Column;
+}
   } else if (Previous.is(TT_InheritanceColon)) {
 CurrentState.Indent = State.Column;
 CurrentState.LastSpace = State.Column;


Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -810,8 +810,9 @@
 // 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)
+Style.BreakBeforeBinaryOperators == FormatStyle::BOS_NonAssignment) {
   CurrentState.LastSpace = State.Column;
+}
   } else if (Previous.is(TT_InheritanceColon)) {
 CurrentState.Indent = State.Column;
 CurrentState.LastSpace = State.Column;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton added a comment.

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.

F24964948: InheritanceArguments.png 

F24964955: Ternary.png 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton added a comment.

> 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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton added a comment.

Adding two inline comments.




Comment at: clang/unittests/Format/FormatTest.cpp:6599-6601
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BinPackParameters = false;
+  Style.ContinuationIndentWidth = 2;

HazardyKnusperkeks wrote:
> Do you need this for the observed effect? From the description I don't see 
> that.
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? 



Comment at: clang/unittests/Format/FormatTest.cpp:6603
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  EXPECT_EQ(
+  "struct Derived {\n"

HazardyKnusperkeks wrote:
> Is the behavior different when the code is already formatted like you want 
> it? (It shouldn't.) Then you should use `verifyFormat()`.
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? 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton updated this revision to Diff 468874.
hel-ableton added a comment.

This should bring back the formerly introduced unit test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6594,6 +6594,32 @@
"(someOtherLongishConditionPart1 || "
"someOtherEvenLongerNestedConditionPart2);",
Style));
+
+  Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BinPackParameters = false;
+  Style.ContinuationIndentWidth = 2;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  EXPECT_EQ(
+  "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"
+  "};",
+  format("struct Derived {"
+ "  Derived(int firstArgWithLongName, int secondArgWithLongName, "
+ "int thirdArgWithLongName, int fourthArgWithLongName)"
+ ": Base(firstArgWithLongName, secondArgWithLongName, "
+ "thirdArgWithLongName, fourthArgWithLongName) {}"
+ "};",
+ Style));
 }
 
 TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -809,8 +809,10 @@
 // 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;


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6594,6 +6594,32 @@
"(someOtherLongishConditionPart1 || "
"someOtherEvenLongerNestedConditionPart2);",
Style));
+
+  Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BinPackParameters = false;
+  Style.ContinuationIndentWidth = 2;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  EXPECT_EQ(
+  "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"
+  "};",
+  format("struct Derived {"
+ "  Derived(int firstArgWithLongName, int secondArgWithLongName, "
+ "int thirdArgWithLongName, int fourthArgWithLongName)"
+ ": Base(firstArgWithLongName, secondArgWithLongName, "
+ "thirdArgWithLongName, fourthArgWithLongName) {}"
+ "};",
+ Style));
 }
 
 TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -809,8 +809,10 @@
 // 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 = Sta

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton added a comment.

> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton updated this revision to Diff 468878.
hel-ableton added a comment.

Using verifyFormat() now instead of EXPECT_EQ()


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6594,7 +6594,25 @@
"(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();
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -809,8 +809,10 @@
 // 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;


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6594,7 +6594,25 @@
"(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();
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -809,8 +809,10 @@
 // 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;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton marked an inline comment as done.
hel-ableton added a comment.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton updated this revision to Diff 468907.
hel-ableton added a comment.

Fixed missing newline.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6594,6 +6594,25 @@
"(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) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -800,8 +800,9 @@
  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)) ||
@@ -809,8 +810,10 @@
 // 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;


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6594,6 +6594,25 @@
"(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) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -800,8 +800,9 @@
  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)) ||
@@ -809,8 +810,10 @@
 // Indent relative to the RHS of the expre

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton marked an inline comment as done.
hel-ableton added a comment.

> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton added a comment.

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. :-)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton marked an inline comment as done.
hel-ableton added a comment.

> 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...




Comment at: clang/lib/Format/ContinuationIndenter.cpp:814
+if (Style.BreakBeforeBinaryOperators == FormatStyle::BOS_None ||
+Style.BreakBeforeBinaryOperators == FormatStyle::BOS_NonAssignment) {
   CurrentState.LastSpace = State.Column;

MyDeveloperDay wrote:
> ok so now you don't actually have a test for this, so I don't know if this is 
> needed any more.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton marked an inline comment as done.
hel-ableton added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton added a comment.

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: +49 30 288 763-256

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



Von: Owen Pan via Phabricator 
Gesendet: Donnerstag, 14. September 2023 02:37
An: Henrik Lafrenz ; developm...@jonas-toth.eu 
; owenpi...@gmail.com ; 
llvm-phrabrica...@hazardy.de ; 
emilia@rymiel.space ; mydeveloper...@gmail.com 

Cc: cfe-commits@lists.llvm.org ; 473750...@qq.com 
<473750...@qq.com>; f_nay...@apple.com ; 
yronglin...@gmail.com ; bhuvanendra.kum...@amd.com 
; 1135831...@qq.com <1135831...@qq.com>; 
gandhi21...@gmail.com ; tbae...@redhat.com 
; mlek...@skidmore.edu ; 
blitzrak...@gmail.com ; shen...@google.com 

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits