[clang-format] [PR45639] clang-format splits up the brackets of C++17 attribute [[ ]] when used with the first parameter

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=45639

clang-format incorrectly splits the [[ in a long argument list

void SomeLongClassName::ALongMethodNameInThatClass([[maybe_unused]] const shared_ptr<ALongTypeName>& argumentNameForThat
LongType) {

}

becomes

void SomeLongClassName::ALongMethodNameInThatClass([
    [maybe_unused]] const shared_ptr<ALongTypeName> &argumentNameForThatLongType) {

}

leaving one [ on the previous line

For a function with just 1 very long argument, clang-format chooses to split between the [[,

This revision adjusts the penalty between ( and [ to ensure this is the more likely break point

[krasimir]

I think we should never break apart the two [[ and ]] of attributes.
Running that example with -debug shows that we consider breaking between the two [ tokens a valid possibility.

• C=1 means breaking before the token is OK, which is probably computed by TokenAnnotator::canBreakBefore:

% bin/clang-format -debug ~/test.cc
Line(0, FSC=0): void[T=85, OC=0] identifier[T=85, OC=5] coloncolon[T=85, OC=22] identifier[T=85, OC=24] l_paren[T=85, OC=50] l_square[T=85, OC=51] l_square[T=85, OC=52] identifier[T=85, OC=53] r_square[T=85, OC=65] r_square[T=85, OC=66] const[T=85, OC=68] identifier[T=85, OC=74] less[T=85, OC=84] identifier[T=85, OC=85] greater[T=85, OC=98] amp[T=85, OC=99] identifier[T=85, OC=101] identifier[T=85, OC=51] r_paren[T=85, OC=59] l_brace[T=21, OC=61] 
Line(0, FSC=0): r_brace[T=85, OC=0] 
Line(0, FSC=0): eof[T=85, OC=0] 
Run 0...
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=void L=4 PPK=2 FakeLParens= FakeRParens=0 II=0x219c910 Text='void'
 M=0 C=1 T=FunctionDeclarationName S=1 B=0 BK=0 P=80 Name=identifier L=22 PPK=2 FakeLParens= FakeRParens=0 II=0x21d8210 Text='SomeLongClassName'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=coloncolon L=24 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='::'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=520 Name=identifier L=50 PPK=2 FakeLParens= FakeRParens=0 II=0x21d8258 Text='ALongMethodNameInThatClass'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=51 PPK=1 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=AttributeSquare S=0 B=0 BK=0 P=140 Name=l_square L=52 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='['
 M=0 C=1 T=AttributeSquare S=0 B=0 BK=0 P=79 Name=l_square L=53 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='['
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=99 Name=identifier L=65 PPK=2 FakeLParens= FakeRParens=0 II=0x21d8290 Text='maybe_unused'
 M=0 C=0 T=AttributeSquare S=0 B=0 BK=0 P=83 Name=r_square L=66 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
 M=0 C=0 T=AttributeSquare S=0 B=0 BK=0 P=63 Name=r_square L=67 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=43 Name=const L=73 PPK=2 FakeLParens= FakeRParens=0 II=0x219c3e8 Text='const'
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=43 Name=identifier L=84 PPK=2 FakeLParens= FakeRParens=0 II=0x21d82c8 Text='shared_ptr'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=50 Name=less L=85 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=380 Name=identifier L=98 PPK=2 FakeLParens= FakeRParens=0 II=0x21d8300 Text='ALongTypeName'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=290 Name=greater L=99 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='>'
 M=0 C=1 T=PointerOrReference S=1 B=0 BK=0 P=230 Name=amp L=101 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='&'
 M=0 C=1 T=StartOfName S=0 B=0 BK=0 P=240 Name=identifier L=120 PPK=2 FakeLParens= FakeRParens=0 II=0x21d8340 Text='argumentNameForThat'
 M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=150 Name=identifier L=129 PPK=2 FakeLParens= FakeRParens=0 II=0x21d8378 Text='LongType'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=43 Name=r_paren L=130 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=FunctionLBrace S=1 B=0 BK=1 P=23 Name=l_brace L=132 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='{'
----

You could adapt that code to not break before the second TT_AttributeSquare in [[ or ]]; this will have the same effect and generalize.

[MyDeveloperDay]

You could adapt that code to not break before the second TT_AttributeSquare in [[ or ]]; this will have the same effect and generalize.

sounds good... let me try that change and see if we like it better

[MyDeveloperDay]

Simplify to ensure we don't break in between the two TT_AttributeSquares

[krasimir]

Thank you!

clang/lib/Format/TokenAnnotator.cpp

bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
   // Allow breaking after a trailing annotation, e.g. after a method
   // declaration.
   if (Left.is(TT_TrailingAnnotation))
     return !Right.isOneOf(tok::l_brace, tok::semi, tok::equal, tok::l_paren,
                           tok::less, tok::coloncolon);
 
   if (Right.is(tok::kw___attribute) ||
       (Right.is(tok::l_square) && Right.is(TT_AttributeSquare)))
-    return true;
+    return !Left.is(TT_AttributeSquare);
 
   if (Left.is(tok::identifier) && Right.is(tok::string_literal))
     return true;
 
   if (Right.is(tok::identifier) && Right.Next && Right.Next->is(TT_DictLiteral))
     return true;
 
   if (Left.is(TT_CtorInitializerColon))

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsSquareAttributes) {
   FormatStyle MultiLineFunctions = getLLVMStyle();
   MultiLineFunctions.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
   verifyFormat("[[unused]] int b() {\n"
                "  return 42;\n"
                "}\n",
                MultiLineFunctions);
 }
 
+TEST_F(FormatTest, AttributePenaltyBreaking) {
+  FormatStyle Style = getLLVMStyle();
+  verifyFormat("void ABCDEFGH::ABCDEFGHIJKLMN(\n"
+               "    [[maybe_unused]] const shared_ptr<ALongTypeName> &C d) {}",
+               Style);
+  verifyFormat("void ABCDEFGH::ABCDEFGHIJK(\n"
+               "    [[maybe_unused]] const shared_ptr<ALongTypeName> &C d) {}",
+               Style);
+  verifyFormat("void ABCDEFGH::ABCDEFGH([[maybe_unused]] const "
+               "shared_ptr<ALongTypeName> &C d) {\n}",
+               Style);
+}
+
 TEST_F(FormatTest, UnderstandsEllipsis) {
   verifyFormat("int printf(const char *fmt, ...);");
   verifyFormat("template <class... Ts> void Foo(Ts... ts) { Foo(ts...); }");
   verifyFormat("template <class... Ts> void Foo(Ts *... ts) {}");
 
   FormatStyle PointersLeft = getLLVMStyle();
   PointersLeft.PointerAlignment = FormatStyle::PAS_Left;
   verifyFormat("template <class... Ts> void Foo(Ts*... ts) {}", PointersLeft);