[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Typz marked 2 inline comments as done.
Closed by commit rG3d61b1120e82: clang-format: Introduce stricter AlignOperands 
flag (authored by Typz).

Changed prior to commit:
  https://reviews.llvm.org/D32478?vs=226393&id=259307#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D32478

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestJS.cpp

Index: clang/unittests/Format/FormatTestJS.cpp
===
--- clang/unittests/Format/FormatTestJS.cpp
+++ clang/unittests/Format/FormatTestJS.cpp
@@ -277,7 +277,7 @@
   verifyFormat("var x = a() in\n"
".aa.aa;");
   FormatStyle Style = getGoogleJSStyleWithColumns(80);
-  Style.AlignOperands = true;
+  Style.AlignOperands = FormatStyle::OAS_Align;
   verifyFormat("var x = a() in\n"
".aa.aa;",
Style);
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4240,6 +4240,9 @@
"  > c) {\n"
"}",
Style);
+  verifyFormat("return a\n"
+   "   && bbb;",
+   Style);
   verifyFormat("return (a)\n"
"   // comment\n"
"   + b;",
@@ -4268,7 +4271,7 @@
 
   Style.ColumnLimit = 60;
   verifyFormat("zz\n"
-   "= b\n"
+   "= \n"
"  >> (aa);",
Style);
 
@@ -4277,7 +4280,7 @@
   Style.TabWidth = 4;
   Style.UseTab = FormatStyle::UT_Always;
   Style.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
-  Style.AlignOperands = false;
+  Style.AlignOperands = FormatStyle::OAS_DontAlign;
   EXPECT_EQ("return someVeryVeryLongConditionThatBarelyFitsOnALine\n"
 "\t&& (someOtherLongishConditionPart1\n"
 "\t\t|| someOtherEvenLongerNestedConditionPart2);",
@@ -4287,6 +4290,107 @@
Style));
 }
 
+TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  Style.AlignOperands = FormatStyle::OAS_AlignAfterOperator;
+
+  verifyFormat("bool value = a\n"
+   "   + a\n"
+   "   + a\n"
+   "  == a\n"
+   " * b\n"
+   " + b\n"
+   "  && a\n"
+   " * a\n"
+   " > c;",
+   Style);
+  verifyFormat("if (a\n"
+   "* \n"
+   "+ aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "+ \n"
+   "  * aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "== \n"
+   "   * aa\n"
+   "   + bbb) {\n}",
+   Style);
+  verifyFormat("if () {\n"
+   "} else if (a\n"
+   "   && b // break\n"
+   "  > c) {\n"
+   "}",
+   Style);
+  verifyFormat("return a\n"
+   "&& b

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.
This revision is now accepted and ready to land.

I've read back through the previous comments, and I'm slightly struggling to 
understand the reason for the objections. (other than the normal public style)

I'd tend to be wary of the "if its not in a public style guide, its not coming 
in" rule despite me understanding the rationale for that stance many people 
have non-public projects but still have millions of lines of code to support:

Many public projects have often already adopted a .clang-format file anyway and 
as such their style is defined by only what clang-format currently supports and 
hence it cannot by definition then define a style that clang-format keeps 
altering the code away from.

So I baulk at the idea that a rule that introduces a new style can't come in, 
My rule of thumb is don't mess up anyone's existing code, don't make others pay 
too much for your feature, help us look after this excellent tool by helping 
the project (bugs, reviews).

With that in mind, I've gone back to the original documentation for 
AlignOperands and BreakBeforeBinaryOperators, and I'm afraid I cannot see the 
logic in the current capabilities that align code nicely with 
BreakBeforeBinaryOperators turned off

F10398828: image.png 

but not nicely with BreakBeforeBinaryOperators turned on

F10398823: image.png 

If I understand correctly, this new setting (which will be off by default) will 
allow the following for those that choose it.

F10398841: image.png 

and it may be the OCD in me, but that re-alignment makes me feel happy, for 
this reason alone I'd give this a LGTM, as long as your willing to help us 
support this behaviour going forward.

That's my rationale for accepting, plus the authors 2.5 years of rebasing a 
feature, shows a dedication which I am assuming extends to continued support.

my 2c worth.

(images from https://zed0.co.uk/clang-format-configurator/) - 10.0.0+b452de0




Comment at: clang/include/clang/Format/Format.h:187
   /// expressions.
-  ///
-  /// Specifically, this aligns operands of a single expression that needs to 
be
-  /// split over multiple lines, e.g.:
-  /// \code
-  ///   int aaa = bbb +
-  /// ccc;
-  /// \endcode
-  bool AlignOperands;
+  OperandAlignmentStyle AlignOperands;
 

Typz wrote:
> MyDeveloperDay wrote:
> > I think you are missing an entry in the operator== in Format.h
> It is there already, since this is not a new field, but just changing the 
> type of an existing field.
sound good.



Comment at: clang/unittests/Format/FormatTest.cpp:4147
"  >> (aa);",
Style);
 

Nit: I get nervous when we change tests, can we simply add a new duplicated line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz marked 2 inline comments as done.
Typz added inline comments.



Comment at: clang/include/clang/Format/Format.h:187
   /// expressions.
-  ///
-  /// Specifically, this aligns operands of a single expression that needs to 
be
-  /// split over multiple lines, e.g.:
-  /// \code
-  ///   int aaa = bbb +
-  /// ccc;
-  /// \endcode
-  bool AlignOperands;
+  OperandAlignmentStyle AlignOperands;
 

MyDeveloperDay wrote:
> I think you are missing an entry in the operator== in Format.h
It is there already, since this is not a new field, but just changing the type 
of an existing field.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/include/clang/Format/Format.h:187
   /// expressions.
-  ///
-  /// Specifically, this aligns operands of a single expression that needs to 
be
-  /// split over multiple lines, e.g.:
-  /// \code
-  ///   int aaa = bbb +
-  /// ccc;
-  /// \endcode
-  bool AlignOperands;
+  OperandAlignmentStyle AlignOperands;
 

I think you are missing an entry in the operator== in Format.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 226393.
Typz added a comment.

Rebase on top of D50078 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D32478

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestJS.cpp

Index: clang/unittests/Format/FormatTestJS.cpp
===
--- clang/unittests/Format/FormatTestJS.cpp
+++ clang/unittests/Format/FormatTestJS.cpp
@@ -277,7 +277,7 @@
   verifyFormat("var x = a() in\n"
".aa.aa;");
   FormatStyle Style = getGoogleJSStyleWithColumns(80);
-  Style.AlignOperands = true;
+  Style.AlignOperands = FormatStyle::OAS_Align;
   verifyFormat("var x = a() in\n"
".aa.aa;",
Style);
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4111,6 +4111,9 @@
"  > c) {\n"
"}",
Style);
+  verifyFormat("return a\n"
+   "   && bbb;",
+   Style);
   verifyFormat("return (a)\n"
"   // comment\n"
"   + b;",
@@ -4139,7 +4142,7 @@
 
   Style.ColumnLimit = 60;
   verifyFormat("zz\n"
-   "= b\n"
+   "= \n"
"  >> (aa);",
Style);
 
@@ -4148,7 +4151,7 @@
   Style.TabWidth = 4;
   Style.UseTab = FormatStyle::UT_Always;
   Style.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
-  Style.AlignOperands = false;
+  Style.AlignOperands = FormatStyle::OAS_DontAlign;
   EXPECT_EQ("return someVeryVeryLongConditionThatBarelyFitsOnALine\n"
 "\t&& (someOtherLongishConditionPart1\n"
 "\t\t|| someOtherEvenLongerNestedConditionPart2);",
@@ -4158,6 +4161,107 @@
Style));
 }
 
+TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  Style.AlignOperands = FormatStyle::OAS_AlignAfterOperator;
+
+  verifyFormat("bool value = a\n"
+   "   + a\n"
+   "   + a\n"
+   "  == a\n"
+   " * b\n"
+   " + b\n"
+   "  && a\n"
+   " * a\n"
+   " > c;",
+   Style);
+  verifyFormat("if (a\n"
+   "* \n"
+   "+ aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "+ \n"
+   "  * aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "== \n"
+   "   * aa\n"
+   "   + bbb) {\n}",
+   Style);
+  verifyFormat("if () {\n"
+   "} else if (a\n"
+   "   && b // break\n"
+   "  > c) {\n"
+   "}",
+   Style);
+  verifyFormat("return a\n"
+   "&& bbb;",
+   Style);
+  verifyFormat("return (a)\n"
+   " // comment\n"
+   " + b;",
+   Style);
+  verif

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 203987.
Typz added a comment.
Herald added a project: clang.

Rebase


Repository:
  rC Clang

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

https://reviews.llvm.org/D32478

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/ContinuationIndenter.h
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestJS.cpp

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -277,7 +277,7 @@
   verifyFormat("var x = a() in\n"
".aa.aa;");
   FormatStyle Style = getGoogleJSStyleWithColumns(80);
-  Style.AlignOperands = true;
+  Style.AlignOperands = FormatStyle::OAS_Align;
   verifyFormat("var x = a() in\n"
".aa.aa;",
Style);
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3837,6 +3837,9 @@
"  > c) {\n"
"}",
Style);
+  verifyFormat("return a\n"
+   "   && bbb;",
+   Style);
   verifyFormat("return (a)\n"
"   // comment\n"
"   + b;",
@@ -3865,7 +3868,7 @@
 
   Style.ColumnLimit = 60;
   verifyFormat("zz\n"
-   "= b\n"
+   "= \n"
"  >> (aa);",
Style);
 
@@ -3874,7 +3877,7 @@
   Style.TabWidth = 4;
   Style.UseTab = FormatStyle::UT_Always;
   Style.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
-  Style.AlignOperands = false;
+  Style.AlignOperands = FormatStyle::OAS_DontAlign;
   EXPECT_EQ("return someVeryVeryLongConditionThatBarelyFitsOnALine\n"
 "\t&& (someOtherLongishConditionPart1\n"
 "\t\t|| someOtherEvenLongerNestedConditionPart2);",
@@ -3882,6 +3885,98 @@
Style));
 }
 
+TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  Style.AlignOperands = FormatStyle::OAS_AlignAfterOperator;
+
+  verifyFormat("bool value = a\n"
+   "   + a\n"
+   "   + a\n"
+   "  == a\n"
+   " * b\n"
+   " + b\n"
+   "  && a\n"
+   " * a\n"
+   " > c;",
+   Style);
+  verifyFormat("if (a\n"
+   "* \n"
+   "+ aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "+ \n"
+   "  * aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "== \n"
+   "   * aa\n"
+   "   + bbb) {\n}",
+   Style);
+  verifyFormat("if () {\n"
+   "} else if (a\n"
+   "   && b // break\n"
+   "  > c) {\n"
+   "}",
+   Style);
+  verifyFormat("return a\n"
+   "&& bbb;",
+   Style);
+  verifyFormat("return (a)\n"
+   " // comment\n"
+   " + b;",
+   Style);
+  verifyFormat(
+  "int aa = aa\n"
+  "   * b

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 158302.
Typz added a comment.
Herald added a subscriber: acoomans.

rebase


Repository:
  rC Clang

https://reviews.llvm.org/D32478

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/ContinuationIndenter.h
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestJS.cpp

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -267,7 +267,7 @@
   verifyFormat("var x = a() in\n"
".aa.aa;");
   FormatStyle Style = getGoogleJSStyleWithColumns(80);
-  Style.AlignOperands = true;
+  Style.AlignOperands = FormatStyle::OAS_Align;
   verifyFormat("var x = a() in\n"
".aa.aa;",
Style);
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3344,6 +3344,9 @@
"  > c) {\n"
"}",
Style);
+  verifyFormat("return a\n"
+   "   && bbb;",
+   Style);
   verifyFormat("return (a)\n"
"   // comment\n"
"   + b;",
@@ -3372,11 +3375,103 @@
 
   Style.ColumnLimit = 60;
   verifyFormat("zz\n"
-   "= b\n"
+   "= \n"
"  >> (aa);",
Style);
 }
 
+TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  Style.AlignOperands = FormatStyle::OAS_AlignAfterOperator;
+
+  verifyFormat("bool value = a\n"
+   "   + a\n"
+   "   + a\n"
+   "  == a\n"
+   " * b\n"
+   " + b\n"
+   "  && a\n"
+   " * a\n"
+   " > c;",
+   Style);
+  verifyFormat("if (a\n"
+   "* \n"
+   "+ aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "+ \n"
+   "  * aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "== \n"
+   "   * aa\n"
+   "   + bbb) {\n}",
+   Style);
+  verifyFormat("if () {\n"
+   "} else if (a\n"
+   "   && b // break\n"
+   "  > c) {\n"
+   "}",
+   Style);
+  verifyFormat("return a\n"
+   "&& bbb;",
+   Style);
+  verifyFormat("return (a)\n"
+   " // comment\n"
+   " + b;",
+   Style);
+  verifyFormat(
+  "int aa = aa\n"
+  "   * bbb\n"
+  "   + cc;",
+  Style);
+
+  verifyFormat("a\n"
+   "=  + ;",
+   Style);
+
+  verifyFormat("return boost::fusion::at_c<0>().second\n"
+   "== boost::fusion::at_c<1>().second;",
+   Style);
+
+  Style.ColumnLimit = 60;
+  verifyFormat("z\n"
+   "= \n"
+ 

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 147031.
Typz added a comment.

Rebase


Repository:
  rC Clang

https://reviews.llvm.org/D32478

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/ContinuationIndenter.h
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestJS.cpp

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -267,7 +267,7 @@
   verifyFormat("var x = a() in\n"
".aa.aa;");
   FormatStyle Style = getGoogleJSStyleWithColumns(80);
-  Style.AlignOperands = true;
+  Style.AlignOperands = FormatStyle::OAS_Align;
   verifyFormat("var x = a() in\n"
".aa.aa;",
Style);
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3319,6 +3319,9 @@
"  > c) {\n"
"}",
Style);
+  verifyFormat("return a\n"
+   "   && bbb;",
+   Style);
   verifyFormat("return (a)\n"
"   // comment\n"
"   + b;",
@@ -3347,11 +3350,103 @@
 
   Style.ColumnLimit = 60;
   verifyFormat("zz\n"
-   "= b\n"
+   "= \n"
"  >> (aa);",
Style);
 }
 
+TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  Style.AlignOperands = FormatStyle::OAS_AlignAfterOperator;
+
+  verifyFormat("bool value = a\n"
+   "   + a\n"
+   "   + a\n"
+   "  == a\n"
+   " * b\n"
+   " + b\n"
+   "  && a\n"
+   " * a\n"
+   " > c;",
+   Style);
+  verifyFormat("if (a\n"
+   "* \n"
+   "+ aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "+ \n"
+   "  * aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "== \n"
+   "   * aa\n"
+   "   + bbb) {\n}",
+   Style);
+  verifyFormat("if () {\n"
+   "} else if (a\n"
+   "   && b // break\n"
+   "  > c) {\n"
+   "}",
+   Style);
+  verifyFormat("return a\n"
+   "&& bbb;",
+   Style);
+  verifyFormat("return (a)\n"
+   " // comment\n"
+   " + b;",
+   Style);
+  verifyFormat(
+  "int aa = aa\n"
+  "   * bbb\n"
+  "   + cc;",
+  Style);
+
+  verifyFormat("a\n"
+   "=  + ;",
+   Style);
+
+  verifyFormat("return boost::fusion::at_c<0>().second\n"
+   "== boost::fusion::at_c<1>().second;",
+   Style);
+
+  Style.ColumnLimit = 60;
+  verifyFormat("z\n"
+   "= \n"
+   "   >> (aaa

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D32478#920345, @Typz wrote:

> > Unless I'm missing something, I'd agree with Daniel; this is not a rule 
> > that's widely used, and I'd say reformatting a code base to the 
> > clang-formatted variant will not regress readability.
>
> Unfortunately coding rules are not just about readability, but also about 
> consistency at entreprise scale, thus as a general rule we should not change 
> the established rules in an organizatoin.
>  There is history in rules, we have code which already uses these rules, so 
> for consistency we must stick to the old rules (even if we would not 
> necessarily choose the same rules if we were to start from scratch).


My guess is (but I might be wrong): if your code base is large enough and you 
search your internal code base for how well these rules are obeyed, I'd expect 
there to be a large number of violations (given that there is no automation to 
detect or fix those violations). In the end, getting actual consistency will 
require effort - either changing all violations, or changing everything to a 
consistent state with automation.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> Unless I'm missing something, I'd agree with Daniel; this is not a rule 
> that's widely used, and I'd say reformatting a code base to the 
> clang-formatted variant will not regress readability.

Unfortunately coding rules are not just about readability, but also about 
consistency at entreprise scale, thus as a general rule we should not change 
the established rules in an organizatoin.
There is history in rules, we have code which already uses these rules, so for 
consistency we must stick to the old rules (even if we would not necessarily 
choose the same rules if we were to start from scratch).


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D32478#920275, @Typz wrote:

> > Sorry for the long response time. I don't see this style rule expressed 
> > explicitly in the Skia or QtCreator style guides - is this something that 
> > just happens to be done sometimes in the code bases?
>
> This is present in code base. Those project's style guides are not precise 
> enough, and do not document the behavior for this case.
>
> > I have problems understanding the rules from the discussion (as well as the 
> > code / test tbh), is there a really concise way to give me the core of the 
> > idea?
>
> The case I am trying to address is to really keep the _operands_ aligned 
> after an assignment or `return`, in case line is wrapped *before* the 
> operator.
>
>   int a = b
> + c;
>   return a
>+ b;
>   
>
> while the current behavior with `Style.AlignOperands = true; 
> BreakBeforeBinaryOperators = true` is to align the wrapped operator with the 
> previous line's operand:
>
>   int a = b
>   + c;
>   return a
>  + b;
>   
>
> In the discussion, it appeared that this behavior is not a error (with 
> respect to the name), but an expected behavior for most coding rules: hence 
> the introduction of a third AlignOperands mode ("AlignAfterOperator"), to 
> handle this new case.
>
> From there the code actually got a bit more complex to support various corner 
> cases (e.g. operators with different number of characters, wrapperd first 
> line, do not unindent more than the enclosing brace...)


Checking out skia and looking at wrapped '+', I see various different formats, 
and random sampling I've found none so far that would fit your proposed rule. 
Unless I'm missing something, I'd agree with Daniel; this is not a rule that's 
widely used, and I'd say reformatting a code base to the clang-formatted 
variant will not regress readability.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> Sorry for the long response time. I don't see this style rule expressed 
> explicitly in the Skia or QtCreator style guides - is this something that 
> just happens to be done sometimes in the code bases?

This is present in code base. Those project's style guides are not precise 
enough, and do not document the behavior for this case.

> I have problems understanding the rules from the discussion (as well as the 
> code / test tbh), is there a really concise way to give me the core of the 
> idea?

The case I am trying to address is to really keep the _operands_ aligned after 
an assignment or `return`, in case line is wrapped *before* the operator.

  int a = b
+ c;
  return a
   + b;

while the current behavior with `Style.AlignOperands = true; 
BreakBeforeBinaryOperators = true` is to align the wrapped operator with the 
previous line's operand:

  int a = b
  + c;
  return a
 + b;

In the discussion, it appeared that this behavior is not a error (with respect 
to the name), but an expected behavior for most coding rules: hence the 
introduction of a third AlignOperands mode ("AlignAfterOperator"), to handle 
this new case.

From there the code actually got a bit more complex to support various corner 
cases (e.g. operators with different number of characters, wrapperd first line, 
do not unindent more than the enclosing brace...)


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D32478#806757, @Typz wrote:

> I renamed the option to `AlignAfterOperator`, is it acceptable now?
>  (I also thought of `UnindentOperator`, but I think it is better to keep the 
> notion of alignment)
>
> Note that this style is used in multiple open-source projects: Skia 
> , parts of QtCreator 
>  code base 
> (e.g. in multiple plugins: clang, cpptools, android...), the OpenEXR 
>  library...


Sorry for the long response time. I don't see this style rule expressed 
explicitly in the Skia or QtCreator style guides - is this something that just 
happens to be done sometimes in the code bases? I have problems understanding 
the rules from the discussion (as well as the code / test tbh), is there a 
really concise way to give me the core of the idea?


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

I renamed the option to `AlignAfterOperator`, is it acceptable now?
(I also thought of `UnindentOperator`, but I think it is better to keep the 
notion of alignment)

Note that this style is used in multiple open-source projects: Skia 
, parts of QtCreator 
 code base 
(e.g. in multiple plugins: clang, cpptools, android...), the OpenEXR 
 library...


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 106221.
Typz marked 3 inline comments as done.
Typz added a comment.

Rename option to AlignAfterOperator


https://reviews.llvm.org/D32478

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/ContinuationIndenter.h
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestJS.cpp

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -102,7 +102,7 @@
   verifyFormat("var x = a() in\n"
".aa.aa;");
   FormatStyle Style = getGoogleJSStyleWithColumns(80);
-  Style.AlignOperands = true;
+  Style.AlignOperands = FormatStyle::OAS_Align;
   verifyFormat("var x = a() in\n"
".aa.aa;",
Style);
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2702,6 +2702,9 @@
"  > c) {\n"
"}",
Style);
+  verifyFormat("return a\n"
+   "   && bbb;",
+   Style);
   verifyFormat("return (a)\n"
"   // comment\n"
"   + b;",
@@ -2730,11 +2733,103 @@
 
   Style.ColumnLimit = 60;
   verifyFormat("zz\n"
-   "= b\n"
+   "= \n"
"  >> (aa);",
Style);
 }
 
+TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  Style.AlignOperands = FormatStyle::OAS_AlignAfterOperator;
+
+  verifyFormat("bool value = a\n"
+   "   + a\n"
+   "   + a\n"
+   "  == a\n"
+   " * b\n"
+   " + b\n"
+   "  && a\n"
+   " * a\n"
+   " > c;",
+   Style);
+  verifyFormat("if (a\n"
+   "* \n"
+   "+ aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "+ \n"
+   "  * aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "== \n"
+   "   * aa\n"
+   "   + bbb) {\n}",
+   Style);
+  verifyFormat("if () {\n"
+   "} else if (a\n"
+   "   && b // break\n"
+   "  > c) {\n"
+   "}",
+   Style);
+  verifyFormat("return a\n"
+   "&& bbb;",
+   Style);
+  verifyFormat("return (a)\n"
+   " // comment\n"
+   " + b;",
+   Style);
+  verifyFormat(
+  "int aa = aa\n"
+  "   * bbb\n"
+  "   + cc;",
+  Style);
+
+  verifyFormat("a\n"
+   "=  + ;",
+   Style);
+
+  verifyFormat("return boost::fusion::at_c<0>().second\n"
+   "== boost::fusion::at_c<1>().second;",
+   Style);
+
+  Style.ColumnLimit = 60;
+  verifyFormat("z\n"
+   "= \n"
+   "   >> aaa

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.



Comment at: include/clang/Format/Format.h:167
+/// \endcode
+OAS_StrictAlign,
+  };

djasper wrote:
> The name is not intuitive. I don't think this is any more or less strict than 
> the other version.
It is a bit stricter in the sense that it really aligns operands, not operator 
with first operand... But this is indeed not very intuitive.

Any suggestion?


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

I don't want to move forward with this patch. But adding Manuel as another 
reviewer to sanity-check.




Comment at: include/clang/Format/Format.h:167
+/// \endcode
+OAS_StrictAlign,
+  };

The name is not intuitive. I don't think this is any more or less strict than 
the other version.



Comment at: unittests/Format/FormatTest.cpp:2781
+  verifyFormat("return (a)\n"
+   "   // comment\n"
+   " + b;",

Comment seems to belong to "+ b" so should be aligned to it.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 103195.
Typz added a comment.

Rebase & fix indentation when newline is inserted after return or equal.


https://reviews.llvm.org/D32478

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/ContinuationIndenter.h
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestJS.cpp

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -102,7 +102,7 @@
   verifyFormat("var x = a() in\n"
".aa.aa;");
   FormatStyle Style = getGoogleJSStyleWithColumns(80);
-  Style.AlignOperands = true;
+  Style.AlignOperands = FormatStyle::OAS_Align;
   verifyFormat("var x = a() in\n"
".aa.aa;",
Style);
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2702,6 +2702,9 @@
"  > c) {\n"
"}",
Style);
+  verifyFormat("return a\n"
+   "   && bbb;",
+   Style);
   verifyFormat("return (a)\n"
"   // comment\n"
"   + b;",
@@ -2730,11 +2733,103 @@
 
   Style.ColumnLimit = 60;
   verifyFormat("zz\n"
-   "= b\n"
+   "= \n"
"  >> (aa);",
Style);
 }
 
+TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  Style.AlignOperands = FormatStyle::OAS_StrictAlign;
+
+  verifyFormat("bool value = a\n"
+   "   + a\n"
+   "   + a\n"
+   "  == a\n"
+   " * b\n"
+   " + b\n"
+   "  && a\n"
+   " * a\n"
+   " > c;",
+   Style);
+  verifyFormat("if (a\n"
+   "* \n"
+   "+ aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "+ \n"
+   "  * aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "== \n"
+   "   * aa\n"
+   "   + bbb) {\n}",
+   Style);
+  verifyFormat("if () {\n"
+   "} else if (a\n"
+   "   && b // break\n"
+   "  > c) {\n"
+   "}",
+   Style);
+  verifyFormat("return a\n"
+   "&& bbb;",
+   Style);
+  verifyFormat("return (a)\n"
+   "   // comment\n"
+   " + b;",
+   Style);
+  verifyFormat(
+  "int aa = aa\n"
+  "   * bbb\n"
+  "   + cc;",
+  Style);
+
+  verifyFormat("a\n"
+   "=  + ;",
+   Style);
+
+  verifyFormat("return boost::fusion::at_c<0>().second\n"
+   "== boost::fusion::at_c<1>().second;",
+   Style);
+
+  Style.ColumnLimit = 60;
+  verifyFormat("z\n"
+   "= \n"
+   "   >> (a

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz marked 7 inline comments as done.
Typz added a comment.

This style is used in the Skia  project.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> Ah damn. Didn't think about the precedences. What I wanted was for the 
> highest level to have a one char operator, e.g.
> 
>   bool a = aa //
>  <<    //
>  | c;
>

Almost, but not quite:

  bool a = aa   //
<<  //
 | c;

i.e. the identifiers is indented by 4 characters, then the operator is 
'unindented.

> I would still be interested in a coding style that recommends this format.

We are using this at our compagny, but this is neither public nor open source.
I am sure I saw this style somewhere else, but I cannot remember where...
(This is also the style recommended for webkit's when returning booleans : 
https://webkit.org/code-style-guidelines/#indentation-wrap-bool-op, but 
probably because the continuation indent is 4...
and I am not sure this guide in Boost's Spirit library counts either: 
http://www.boost.org/doc/libs/1_56_0/libs/spirit/doc/html/spirit/notes/style_guide.html
 )


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In https://reviews.llvm.org/D32478#765642, @Typz wrote:

> Nop, it's formatted like this:
>
>   bool a = aa   //
> ==  //
> && c;
>   
>   bool a = aa //
> ==    //
>+ c;
>   
>
> The current way to format operators is not affected: the indentation is done 
> as "usual", then they are unindented by the operator width to keep the 
> alignment...


Ah damn. Didn't think about the precedences. What I wanted was for the highest 
level to have a one char operator, e.g.

  bool a = aa //
 <<    //
 | c;

However, you example is also interesting

In https://reviews.llvm.org/D32478#765548, @Typz wrote:

> In https://reviews.llvm.org/D32478#765537, @djasper wrote:
>
> > In all honesty, I think this style isn't thought out well enough. It really 
> > is a special case for only "=" and "return" and even there, it has many 
> > cases where it simply doesn't make sense. And then you have cases like this:
> >
> >   bool = aa //
> >   ==  //
> >   && c;
> >   
> >
> > Where the syntactic structure is lost entirely.
>
>
> It is not lost, extra indent for 'virtual' parenthesis is still there:
>
>   bool a = aa   //
> ==  //
> && c;
>   
>
> > On top of that it has runtime downsides for all clang-format users because 
> > ParenState gets larger and more costly compare. As such, I am against 
> > moving forward with this. Can you remind me again, which coding style 
> > suggests this format?
>
> This is just a single extra bit (and there are still less than 16 such bits), 
> so it does change the size of ParenState. As for the compare cost, I think it 
> is within reach of the compiler's optimization, but it may indeed have a 
> slight impact.


I would still be interested in a coding style that recommends this format.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Nop, it's formatted like this:

  bool a = aa   //
==  //
&& c;
  
  bool a = aa //
==    //
   + c;


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In https://reviews.llvm.org/D32478#765583, @Typz wrote:

> I actually don't know how, but it still manages somehow : I rebuilt this 
> exact patch to ensure I gave you the correct output.
>  And the same behavior can be seen in the test cases, where the operator with 
> highest precedence is aligned with the equal sign.


So, it's formatting like this?

  bool a = aa   //
==  //
&& c;
  
  auto a = aa//
 ==  //
 + c;

While that's interesting and I'd like to figure out how it actually works, I 
also think it's really weird to indent the inner "==" differently based on the 
kind of operator around it. The existing way to format these operators is nice 
and consistent here with always adding a multiple of four spaces.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

I actually don't know how, but it still manages somehow : I rebuilt this exact 
patch to ensure I gave you the correct output.
And the same behavior can be seen in the test cases, where the operator with 
highest precedence is aligned with the equal sign.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In https://reviews.llvm.org/D32478#765548, @Typz wrote:

> In https://reviews.llvm.org/D32478#765537, @djasper wrote:
>
> > In all honesty, I think this style isn't thought out well enough. It really 
> > is a special case for only "=" and "return" and even there, it has many 
> > cases where it simply doesn't make sense. And then you have cases like this:
> >
> >   bool = aa //
> >   ==  //
> >   && c;
> >   
> >
> > Where the syntactic structure is lost entirely.
>
>
> It is not lost, extra indent for 'virtual' parenthesis is still there:
>
>   bool a = aa   //
> ==  //
> && c;


Ah, right, I was thinking about something else (commas where we don't add extra 
indentation. Anyhow, I don't think what you are writing is what clang-format 
produces. How could it indent relative to the "&&" when placing the "=="? It 
doesn't know how far to unindent at that point, I think.

> 
> 
>> On top of that it has runtime downsides for all clang-format users because 
>> ParenState gets larger and more costly compare. As such, I am against moving 
>> forward with this. Can you remind me again, which coding style suggests this 
>> format?
> 
> This is just a single extra bit (and there are still less than 16 such bits), 
> so it does change the size of ParenState. As for the compare cost, I think it 
> is within reach of the compiler's optimization, but it may indeed have a 
> slight impact.




https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D32478#765537, @djasper wrote:

> In all honesty, I think this style isn't thought out well enough. It really 
> is a special case for only "=" and "return" and even there, it has many cases 
> where it simply doesn't make sense. And then you have cases like this:
>
>   bool = aa //
>   ==  //
>   && c;
>   
>
> Where the syntactic structure is lost entirely.




  bool a = aa   //
==  //
&& c;

> On top of that it has runtime downsides for all clang-format users because 
> ParenState gets larger and more costly compare. As such, I am against moving 
> forward with this. Can you remind me again, which coding style suggests this 
> format?

This is just a single extra bit (and there are still less than 16 such bits), 
so it does change the size of ParenState. As for the compare cost, I think it 
is within reach of the compiler's optimization, but it may indeed have a slight 
impact.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In all honesty, I think this style isn't thought out well enough. It really is 
a special case for only "=" and "return" and even there, it has many cases 
where it simply doesn't make sense. And then you have cases like this:

  bool = aa //
  ==  //
  && c;

Where the syntactic structure is lost entirely.

On top of that it has runtime downsides for all clang-format users because 
ParenState gets larger and more costly compare. As such, I am against moving 
forward with this. Can you remind me again, which coding style suggests this 
format?




Comment at: unittests/Format/FormatTest.cpp:2619
+  "sizeof(int16_t) // DWARF ARange version number\n"
+  "  + sizeof(int32_t) // Offset of CU in the .debug_info section\n"
+  "  + sizeof(int8_t)  // Pointer Size (in bytes)\n"

I think this is wrong and we should not indent like this.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 100384.
Typz added a comment.

fix style


https://reviews.llvm.org/D32478

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/ContinuationIndenter.h
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestJS.cpp

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -102,7 +102,7 @@
   verifyFormat("var x = a() in\n"
".aa.aa;");
   FormatStyle Style = getGoogleJSStyleWithColumns(80);
-  Style.AlignOperands = true;
+  Style.AlignOperands = FormatStyle::OAS_Align;
   verifyFormat("var x = a() in\n"
".aa.aa;",
Style);
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2503,6 +2503,9 @@
"  > c) {\n"
"}",
Style);
+  verifyFormat("return a\n"
+   "   && bbb;",
+   Style);
   verifyFormat("return (a)\n"
"   // comment\n"
"   + b;",
@@ -2531,11 +2534,103 @@
 
   Style.ColumnLimit = 60;
   verifyFormat("zz\n"
-   "= b\n"
+   "= \n"
"  >> (aa);",
Style);
 }
 
+TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  Style.AlignOperands = FormatStyle::OAS_StrictAlign;
+
+  verifyFormat("bool value = a\n"
+   "   + a\n"
+   "   + a\n"
+   "  == a\n"
+   " * b\n"
+   " + b\n"
+   "  && a\n"
+   " * a\n"
+   " > c;",
+   Style);
+  verifyFormat("if (a\n"
+   "* \n"
+   "+ aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "+ \n"
+   "  * aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "== \n"
+   "   * aa\n"
+   "   + bbb) {\n}",
+   Style);
+  verifyFormat("if () {\n"
+   "} else if (a\n"
+   "   && b // break\n"
+   "  > c) {\n"
+   "}",
+   Style);
+  verifyFormat("return a\n"
+   "&& bbb;",
+   Style);
+  verifyFormat("return (a)\n"
+   "   // comment\n"
+   " + b;",
+   Style);
+  verifyFormat(
+  "int aa = aa\n"
+  "   * bbb\n"
+  "   + cc;",
+  Style);
+
+  verifyFormat("a\n"
+   "=  + ;",
+   Style);
+
+  verifyFormat("return boost::fusion::at_c<0>().second\n"
+   "== boost::fusion::at_c<1>().second;",
+   Style);
+
+  Style.ColumnLimit = 60;
+  verifyFormat("z\n"
+   "= \n"
+   "   >> (aa);",
+   Style);
+
+  // Force

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:949
+ Previous->is(tok::kw_return)))
+  NewParenState.UnindentOperator = true;
 

djasper wrote:
> I am not yet convinced you need a new flag in ParenState here. I guess you 
> need it because the operators can have varying length and so you cannot just 
> change LastSpace here?
exactly. This is set when passing through the equal/return sign, but indent 
must be adjusted for each line individually based on the actual size of that 
line's leading operator.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

It indeed does not happens inside any parenthesis (it would actually make 
things completely unreadable if there are imbricated parenthesis), and may get 
indented less than the ContinuationIndentWidth.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In https://reviews.llvm.org/D32478#759347, @Typz wrote:

> In https://reviews.llvm.org/D32478#758258, @djasper wrote:
>
> > When you say "this doesn't happen in tests", do you mean this never happens 
> > when there are parentheses around the expression?
>
>
> By 'test' I meant 'conditional construct' : it happens when there are 
> parentheses around the expression, but it does not happen when these 
> parentheses are the parentheses from a `if (...)`


Are you sure? From reading the code, it seems that this happens exactly after 
"=" and "return". What's the behavior for function calls?

  function(aaa //
  + b);

Or for expressions with just parens?

  int a = (aa //
  + bb);

What if doing this would violate the ContinuationIndentWidth?

  T t = a //
  && b;




Comment at: lib/Format/ContinuationIndenter.cpp:759
+return State.Stack.back().Indent - Current.Tok.getLength()
+- Current.SpacesRequiredBefore;
   if (State.Stack.back().Indent == State.FirstIndent && PreviousNonComment &&

Fix style.



Comment at: lib/Format/ContinuationIndenter.cpp:949
+ Previous->is(tok::kw_return)))
+  NewParenState.UnindentOperator = true;
 

I am not yet convinced you need a new flag in ParenState here. I guess you need 
it because the operators can have varying length and so you cannot just change 
LastSpace here?


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 100095.
Typz added a comment.

add new value to AlignOperands to support this mode.
fix some corner cases.


https://reviews.llvm.org/D32478

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/ContinuationIndenter.h
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestJS.cpp

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -102,7 +102,7 @@
   verifyFormat("var x = a() in\n"
".aa.aa;");
   FormatStyle Style = getGoogleJSStyleWithColumns(80);
-  Style.AlignOperands = true;
+  Style.AlignOperands = FormatStyle::OAS_Align;
   verifyFormat("var x = a() in\n"
".aa.aa;",
Style);
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2503,6 +2503,9 @@
"  > c) {\n"
"}",
Style);
+  verifyFormat("return a\n"
+   "   && bbb;",
+   Style);
   verifyFormat("return (a)\n"
"   // comment\n"
"   + b;",
@@ -2531,11 +2534,104 @@
 
   Style.ColumnLimit = 60;
   verifyFormat("zz\n"
-   "= b\n"
+   "= \n"
"  >> (aa);",
Style);
 }
 
+TEST_F(FormatTest, ExpressionIndentationStrictAlign) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  Style.AlignOperands = FormatStyle::OAS_StrictAlign;
+
+  verifyFormat(
+  "bool value = a\n"
+  "   + a\n"
+  "   + a\n"
+  "  == a\n"
+  " * b\n"
+  " + b\n"
+  "  && a\n"
+  " * a\n"
+  " > c;",
+  Style);
+  verifyFormat("if (a\n"
+   "* \n"
+   "+ aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "+ \n"
+   "  * aa\n"
+   "== bbb) {\n}",
+   Style);
+  verifyFormat("if (a\n"
+   "== \n"
+   "   * aa\n"
+   "   + bbb) {\n}",
+   Style);
+  verifyFormat("if () {\n"
+   "} else if (a\n"
+   "   && b // break\n"
+   "  > c) {\n"
+   "}",
+   Style);
+  verifyFormat("return a\n"
+   "&& bbb;",
+   Style);
+  verifyFormat("return (a)\n"
+   "   // comment\n"
+   " + b;",
+   Style);
+  verifyFormat(
+  "int aa = aa\n"
+  "   * bbb\n"
+  "   + cc;",
+  Style);
+
+  verifyFormat("a\n"
+   "=  + ;",
+   Style);
+
+  verifyFormat("return boost::fusion::at_c<0>().second\n"
+   "== boost::fusion::at_c<1>().second;",
+   Style);
+
+  Style.ColumnLimit = 60;
+  verifyFormat("z\n"
+   "= \n"
+   "   >> (aa);",
+   Style);
+
+  // Forced by co

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.



Comment at: unittests/Format/FormatTest.cpp:2476
   "bool value = a\n"
-  " + a\n"
-  " + a\n"
-  " == a\n"
-  "* b\n"
-  "+ b\n"
-  " && a\n"
-  "* a\n"
-  "> c;",
+  "   + a\n"
+  "   + a\n"

djasper wrote:
> Typz wrote:
> > Typz wrote:
> > > djasper wrote:
> > > > This looks very inconsistent to me.
> > > not sure what you mean, I do not really understand how this expression 
> > > was aligned before the patch...
> > > it is not so much better in this case with the patch, but the '&&' is 
> > > actually right-aligned with the '=' sign.
> > Seeing the test just before, I see (when breaking after operators) that the 
> > operands are actually right-aligned, e.g. all operators are on the same 
> > column.
> > 
> > So should it not be the same when breaking before the operator as well 
> > (independently from my patch, actually)?
> > 
> >   bool value = a\n"
> >  + a\n"
> >  + a\n"
> > == a\n"
> >  * b\n"
> >  + b\n"
> > && a\n"
> >  * a\n"
> >  > c;
> > 
> > Not sure I like this right-alignment thing, but at least I start to 
> > understand how we get this output (and this may be another option to prefer 
> > left-alignment?)
> The right alignment is not relevant. I think I just got "playful" when 
> writing this test.
> 
> What's happening here is that we align the operators of subsequent lines to 
> the previous operand. You are right that this is not intuitive wrt. the 
> option's name, but it is the behavior that we intended and that we had seen 
> in styles that use this.
> 
> Now, what we also do is add another ContinuationIndentWidth to highlight 
> operator precedence. Basically think of this as replacing parentheses that 
> you would put there instead:
> 
>   int x = (a
>* b) // The "*" is intendet a space because of the paren.
>   + c;
> 
>   int x = a
>   * b // Instead we use ContinuationIndentWidth here.
>   + c;
> 
> What I mean about this being inconsistent is that now you align the 
> highest-level operands because as you say that's what's expected from the 
> option, but you still don't align other precedences, which seems wrong. If 
> you really wanted to align all operands, that would look like:
> 
>   bool value = a
>  + a
>  + a
> == a
>  * b
>  + b
> && a
>  * a
>  > c;
> 
> But then, that's really a tough expression to understand. I mean, this is a 
> fabricated example and possibly doesn't happen often in real life, but I am 
> slightly worried about it and the inconsistency here.
So how should we proceed?

As I understand it, there are 2 separate issues here:
 * The current behavior is actually the expected one, and should not be modified
 * The indentation due to 'fake parenthesis' is not very explicit (even with 
the current implementation)

Changing AlignOperands into an OperandAlignment enum, with 3 options 
(`OA_NotAligned`, same as `AlignOperands=false`; `OA_AlignOperator` (or 
OA_Align), same the current `AlignOperands=true`; and `OA_AlignOperands` (or 
OA_StrictAlign), with my new behavior), would solve the first problem. Would it 
be acceptable?

I don't think this is related to the extra indentation issue, the problem is 
already present, and the existing behavior is not that consistent or clear... 
But I think the 'fake parenthesis' indentation could easily be changed, 
independently from 

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D32478#758258, @djasper wrote:

> When you say "this doesn't happen in tests", do you mean this never happens 
> when there are parentheses around the expression?


By 'test' I meant 'conditional construct' : it happens when there are 
parentheses around the expression, but it does not happen when these 
parentheses are the parentheses from a `if (...)`


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

When you say "this doesn't happen in tests", do you mean this never happens 
when there are parentheses around the expression?




Comment at: unittests/Format/FormatTest.cpp:2476
   "bool value = a\n"
-  " + a\n"
-  " + a\n"
-  " == a\n"
-  "* b\n"
-  "+ b\n"
-  " && a\n"
-  "* a\n"
-  "> c;",
+  "   + a\n"
+  "   + a\n"

Typz wrote:
> Typz wrote:
> > djasper wrote:
> > > This looks very inconsistent to me.
> > not sure what you mean, I do not really understand how this expression was 
> > aligned before the patch...
> > it is not so much better in this case with the patch, but the '&&' is 
> > actually right-aligned with the '=' sign.
> Seeing the test just before, I see (when breaking after operators) that the 
> operands are actually right-aligned, e.g. all operators are on the same 
> column.
> 
> So should it not be the same when breaking before the operator as well 
> (independently from my patch, actually)?
> 
>   bool value = a\n"
>  + a\n"
>  + a\n"
> == a\n"
>  * b\n"
>  + b\n"
> && a\n"
>  * a\n"
>  > c;
> 
> Not sure I like this right-alignment thing, but at least I start to 
> understand how we get this output (and this may be another option to prefer 
> left-alignment?)
The right alignment is not relevant. I think I just got "playful" when writing 
this test.

What's happening here is that we align the operators of subsequent lines to the 
previous operand. You are right that this is not intuitive wrt. the option's 
name, but it is the behavior that we intended and that we had seen in styles 
that use this.

Now, what we also do is add another ContinuationIndentWidth to highlight 
operator precedence. Basically think of this as replacing parentheses that you 
would put there instead:

  int x = (a
   * b) // The "*" is intendet a space because of the paren.
  + c;

  int x = a
  * b // Instead we use ContinuationIndentWidth here.
  + c;

What I mean about this being inconsistent is that now you align the 
highest-level operands because as you say that's what's expected from the 
option, but you still don't align other precedences, which seems wrong. If you 
really wanted to align all operands, that would look like:

  bool value = a
 + a
 + a
== a
 * b
 + b
&& a
 * a
 > c;

But then, that's really a tough expression to understand. I mean, this is a 
fabricated example and possibly doesn't happen often in real life, but I am 
slightly worried about it and the inconsistency here.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.



Comment at: unittests/Format/FormatTest.cpp:2476
   "bool value = a\n"
-  " + a\n"
-  " + a\n"
-  " == a\n"
-  "* b\n"
-  "+ b\n"
-  " && a\n"
-  "* a\n"
-  "> c;",
+  "   + a\n"
+  "   + a\n"

Typz wrote:
> djasper wrote:
> > This looks very inconsistent to me.
> not sure what you mean, I do not really understand how this expression was 
> aligned before the patch...
> it is not so much better in this case with the patch, but the '&&' is 
> actually right-aligned with the '=' sign.
Seeing the test just before, I see (when breaking after operators) that the 
operands are actually right-aligned, e.g. all operators are on the same column.

So should it not be the same when breaking before the operator as well 
(independently from my patch, actually)?

  bool value = a\n"
 + a\n"
 + a\n"
== a\n"
 * b\n"
 + b\n"
&& a\n"
 * a\n"
 > c;

Not sure I like this right-alignment thing, but at least I start to understand 
how we get this output (and this may be another option to prefer 
left-alignment?)


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

we are using this style at our company, not sure if it is used elsewhere; I 
will check.

however, it seems to me that this behavior does not match the name of the 
option : AlignOperands does not align the operands anymore when 
BreakBeforeBinaryOperators is set...
so, for this patch to go forward, should I change AlignOperands into 
OperandAlignment enum, with 3 options? Something like

- OA_NotAligned, same as AlignOperands=false
- OA_AlignOperator, same the current AlignOperands=true
- OA_AlignOperands, same as AlignOperands=true when 
BreakBeforeBinaryOperators=false but my "new" mode when 
BreakBeforeBinaryOperators=true




Comment at: unittests/Format/FormatTest.cpp:2476
   "bool value = a\n"
-  " + a\n"
-  " + a\n"
-  " == a\n"
-  "* b\n"
-  "+ b\n"
-  " && a\n"
-  "* a\n"
-  "> c;",
+  "   + a\n"
+  "   + a\n"

djasper wrote:
> This looks very inconsistent to me.
not sure what you mean, I do not really understand how this expression was 
aligned before the patch...
it is not so much better in this case with the patch, but the '&&' is actually 
right-aligned with the '=' sign.


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

The current behavior here is actually intended. If you'd like the other format, 
we probably need to add a style option. What style guide are you basing this on?




Comment at: unittests/Format/FormatTest.cpp:2476
   "bool value = a\n"
-  " + a\n"
-  " + a\n"
-  " == a\n"
-  "* b\n"
-  "+ b\n"
-  " && a\n"
-  "* a\n"
-  "> c;",
+  "   + a\n"
+  "   + a\n"

This looks very inconsistent to me.


https://reviews.llvm.org/D32478



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