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

[daphnediane]

Summary

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.

[djasper]

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.

[daphnediane]

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

[daphnediane]

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

[djasper]

Looks good. Thank you!

[daphnediane]

In 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?

[Eugene.Zelenko]

Looks like patch was not committed.

[daphnediane]

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.

[daphnediane]

ping. I've been using this for a long time and still seems to apply to cleanly and works, though the line numbers have shifted a little since I generated the previous diff.

[djasper]

Submitted as r294179. Sorry I missed this before.

lib/Format/TokenAnnotator.cpp

if ((Left.isBinaryOperator() || Left.is(TT_BinaryOperator)) &&
     return true;
   return Left.isOneOf(tok::comma, tok::coloncolon, tok::semi, tok::l_brace,
                       tok::kw_class, tok::kw_struct, tok::comment) ||
          Right.isMemberAccess() ||
          Right.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow, tok::lessless,
                        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) {
   llvm::errs() << "AnnotatedTokens:\n";
   const FormatToken *Tok = Line.First;
   while (Tok) {
     llvm::errs() << " M=" << Tok->MustBreakBefore
                  << " C=" << Tok->CanBreakBefore

unittests/Format/FormatTest.cpp

verifyFormat("template <typename T> // T can be A, B or C.\n"
                "struct C {};",
                AlwaysBreak);
   verifyFormat("template <enum E> class A {\n"
                "public:\n"
                "  E *f();\n"
                "};");
 }
 
+TEST_F(FormatTest, WrapsTemplateParameters) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
+  verifyFormat(
+      "template <typename... a> struct q {};\n"
+      "extern q<aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaa,\n"
+      "    aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaa>\n"
+      "    y;",
+      Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_DontAlign;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  verifyFormat(
+      "template <typename... a> struct r {};\n"
+      "extern r<aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaa,\n"
+      "    aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaa>\n"
+      "    y;",
+      Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
+  verifyFormat(
+      "template <typename... a> struct s {};\n"
+      "extern s<\n"
+      "    aaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaa,\n"
+      "    aaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaa>\n"
+      "    y;",
+      Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  verifyFormat(
+      "template <typename... a> struct t {};\n"
+      "extern t<\n"
+      "    aaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaa,\n"
+      "    aaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaa>\n"
+      "    y;",
+      Style);
+}
+
 TEST_F(FormatTest, WrapsAtNestedNameSpecifiers) {
   verifyFormat(
       "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa::\n"
       "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa();");
   verifyFormat(
       "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa::\n"
       "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
       "        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa());");