[PATCH] D24703: [clang-format] BreakBeforeBinaryOperations and AlignAfterOpenBracket conflict, bug 30304

[Daphne Pfister via cfe-commits]

daphnediane created this revision.
daphnediane added a reviewer: djasper.
daphnediane added a subscriber: cfe-commits.
Herald added a subscriber: klimek.

Fix for the formatting options combination of BreakBeforeBinaryOperators: All, 
AlignAfterOpenBracket: AlwaysBreak not handling long templates correctly. This 
patch allows a break after an opening left parenthesis, TemplateOpener, or 
bracket when both options are enabled.

https://reviews.llvm.org/D24703

Files:
  lib/Format/TokenAnnotator.cpp

Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2513,9 +2513,11 @@
 return true;
   if ((Left.isBinaryOperator() || Left.is(TT_BinaryOperator)) &&
   !Left.isOneOf(tok::arrowstar, tok::lessless) &&
-  Style.BreakBeforeBinaryOperators != FormatStyle::BOS_All &&
-  (Style.BreakBeforeBinaryOperators == FormatStyle::BOS_None ||
-   Left.getPrecedence() == prec::Assignment))
+  ((Style.AlignAfterOpenBracket == FormatStyle::BAS_AlwaysBreak &&
+Left.isOneOf(tok::l_paren, TT_TemplateOpener, tok::l_square)) ||
+   (Style.BreakBeforeBinaryOperators != FormatStyle::BOS_All &&
+(Style.BreakBeforeBinaryOperators == FormatStyle::BOS_None ||
+ Left.getPrecedence() == prec::Assignment
 return true;
   return Left.isOneOf(tok::comma, tok::coloncolon, tok::semi, tok::l_brace,
   tok::kw_class, tok::kw_struct, tok::comment) ||


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2513,9 +2513,11 @@
 return true;
   if ((Left.isBinaryOperator() || Left.is(TT_BinaryOperator)) &&
   !Left.isOneOf(tok::arrowstar, tok::lessless) &&
-  Style.BreakBeforeBinaryOperators != FormatStyle::BOS_All &&
-  (Style.BreakBeforeBinaryOperators == FormatStyle::BOS_None ||
-   Left.getPrecedence() == prec::Assignment))
+  ((Style.AlignAfterOpenBracket == FormatStyle::BAS_AlwaysBreak &&
+Left.isOneOf(tok::l_paren, TT_TemplateOpener, tok::l_square)) ||
+   (Style.BreakBeforeBinaryOperators != FormatStyle::BOS_All &&
+(Style.BreakBeforeBinaryOperators == FormatStyle::BOS_None ||
+ Left.getPrecedence() == prec::Assignment
 return true;
   return Left.isOneOf(tok::comma, tok::coloncolon, tok::semi, tok::l_brace,
   tok::kw_class, tok::kw_struct, tok::comment) ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24703: [clang-format] BreakBeforeBinaryOperations and AlignAfterOpenBracket conflict, bug 30304

[Daphne Pfister via cfe-commits]

daphnediane added a comment.

In https://reviews.llvm.org/D24703#545706, @djasper wrote:

> I think, this is the wrong fix. Instead, a
>
>   || (Left.is(TT_TemplateOpener) && !Right.is(TT_TemplateCloser))
>   
>
> should be added to the last return statement of this function.
>
> Also, could you please add a test in unittests/Format/FormatTest.cpp.


Trying that on my code after I finish rebasing and rebuilding, will also look 
into adding test to FormatTest.


https://reviews.llvm.org/D24703



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

Re: [PATCH] D24703: [clang-format] BreakBeforeBinaryOperations and AlignAfterOpenBracket conflict, bug 30304

[Daphne Pfister via cfe-commits]

daphnediane updated this revision to Diff 71767.
daphnediane added a comment.

Adds test case, changes to suggested fix allowing break after template opener 
that is not immediately followed by template closer.


https://reviews.llvm.org/D24703

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5403,6 +5403,44 @@
"};");
 }
 
+TEST_F(FormatTest, WrapsTemplateParameters) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
+  verifyFormat(
+  "template  struct q {};\n"
+  "extern q\n"
+  "y;",
+  Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  verifyFormat(
+  "template  struct r {};\n"
+  "extern r\n"
+  "y;",
+  Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
+  verifyFormat(
+  "template  struct s {};\n"
+  "extern s<\n"
+  "aa, aa, 
aa,\n"
+  "aa, aa, 
aa>\n"
+  "y;",
+  Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  verifyFormat(
+  "template  struct t {};\n"
+  "extern t<\n"
+  "aa, aa, 
aa,\n"
+  "aa, aa, 
aa>\n"
+  "y;",
+  Style);
+}
+
 TEST_F(FormatTest, WrapsAtNestedNameSpecifiers) {
   verifyFormat(
   
"::\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2524,7 +2524,8 @@
tok::colon, tok::l_square, tok::at) ||
  (Left.is(tok::r_paren) &&
   Right.isOneOf(tok::identifier, tok::kw_const)) ||
- (Left.is(tok::l_paren) && !Right.is(tok::r_paren));
+ (Left.is(tok::l_paren) && !Right.is(tok::r_paren)) ||
+ (Left.is(TT_TemplateOpener) && !Right.is(TT_TemplateCloser));
 }
 
 void TokenAnnotator::printDebugInfo(const AnnotatedLine &Line) {


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5403,6 +5403,44 @@
"};");
 }
 
+TEST_F(FormatTest, WrapsTemplateParameters) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
+  verifyFormat(
+  "template  struct q {};\n"
+  "extern q\n"
+  "y;",
+  Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  verifyFormat(
+  "template  struct r {};\n"
+  "extern r\n"
+  "y;",
+  Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
+  verifyFormat(
+  "template  struct s {};\n"
+  "extern s<\n"
+  "aa, aa, aa,\n"
+  "aa, aa, aa>\n"
+  "y;",
+  Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  verifyFormat(
+  "template  struct t {};\n"
+  "extern t<\n"
+  "aa, aa, aa,\n"
+  "aa, aa, aa>\n"
+  "y;",
+  Style);
+}
+
 TEST_F(FormatTest, WrapsAtNestedNameSpecifiers) {
   verifyFormat(
   "::\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2524,7 +2524,8 @@
tok::colon, tok::l_square, tok::at) ||
  (Left.is(tok::r_paren) &&
   Right.isOneOf(tok::identifier, tok::kw_const)) ||
- (Left.is(tok::l_paren) && !Right.is(tok::r_paren));
+ (Left.is(tok::l_paren) && !Right.is(tok::r_paren)) ||
+ (Left.is(TT_TemplateOpener) && !Right.is(TT_TemplateCloser));
 }
 
 void TokenAnnotator::printDebugInfo(const A

Re: [PATCH] D24703: [clang-format] BreakBeforeBinaryOperations and AlignAfterOpenBracket conflict, bug 30304

[Daphne Pfister via cfe-commits]

daphnediane added a comment.

In https://reviews.llvm.org/D24703#545917, @djasper wrote:

> Looks good. Thank you!


Thanks. Are there additional steps I need to get it commited or do I just wait 
for a commiter to notice?


https://reviews.llvm.org/D24703



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

[PATCH] D24703: [clang-format] BreakBeforeBinaryOperations and AlignAfterOpenBracket conflict, bug 30304

[Daphne Pfister via cfe-commits]

daphnediane added a comment.

ping.

FYI I have found some additional similar cases when 
AlignConsecutiveDeclarations is also true, that I'll see if I can document/test 
case etc. at some point.


https://reviews.llvm.org/D24703



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