Formatter eats space in front of attributes for `operator delete`

[nico]

Bugzilla Link	26663
Version	trunk
OS	All

Extended Description

This works:

$ echo 'void operator new(void* foo) ATTRIB;' | bin/clang-format -
void operator new(void *foo) ATTRIB;

This doesn't:

$ echo 'void operator delete(void* foo) ATTRIB;' | bin/clang-format -
void operator delete(void *foo)ATTRIB;

Note the missing space in front of ATTRIB.

[nico]

here's a test. i tried for a few minutes to make it pass, but failed:

Index: unittests/Format/FormatTest.cpp
===================================================================
--- unittests/Format/FormatTest.cpp	(revision 261298)
+++ unittests/Format/FormatTest.cpp	(working copy)
@@ -5528,8 +5528,10 @@
   verifyFormat("operator SomeType<int, int>();");
   verifyFormat("operator SomeType<SomeType<int>>();");
   verifyFormat("void *operator new(std::size_t size);");
+  verifyFormat("void *operator new(size_t size) ALIAS(ShimNew);");
   verifyFormat("void *operator new[](std::size_t size);");
   verifyFormat("void operator delete(void *ptr);");
+  verifyFormat("void operator delete(void *ptr) ALIAS(ShimDelete);");
   verifyFormat("void operator delete[](void *ptr);");
   verifyFormat("template <typename AAAAAAA, typename BBBBBBB>\n"
                "AAAAAAA operator/(const AAAAAAA &a, BBBBBBB &b);");

[nico]

clang-format -debug output for operator delete version:

 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=void L=4 PPK=2 FakeLParens= FakeRParens=0 II=0x7fe02d004fd0 Text='void'
 M=0 C=1 T=FunctionDeclarationName S=1 B=0 BK=0 P=220 Name=operator L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x7fe02d0055c8 Text='operator'
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=23 Name=delete L=20 PPK=2 FakeLParens= FakeRParens=0 II=0x7fe02d0053f8 Text='delete'
 M=0 C=0 T=OverloadedOperatorLParen S=0 B=0 BK=0 P=23 Name=l_paren L=21 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=void L=25 PPK=2 FakeLParens= FakeRParens=0 II=0x7fe02d004fd0 Text='void'
 M=0 C=0 T=PointerOrReference S=0 B=0 BK=0 P=230 Name=star L=26 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='*'
 M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=240 Name=identifier L=30 PPK=2 FakeLParens= FakeRParens=0 II=0x7fe02d00aa00 Text='foo'
 M=0 C=0 T=CastRParen S=0 B=0 BK=0 P=43 Name=r_paren L=31 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=1 T=TrailingAnnotation S=0 B=0 BK=0 P=120 Name=identifier L=37 PPK=2 FakeLParens= FakeRParens=0 II=0x7fe02d00aa30 Text='ATTRIB'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=38 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

For operator delete2 version:

 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=void L=4 PPK=2 FakeLParens= FakeRParens=0 II=0x7fc8c9804fd0 Text='void'
 M=0 C=1 T=Unknown S=1 B=0 BK=0 P=220 Name=operator L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x7fc8c98055c8 Text='operator'
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=23 Name=identifier L=21 PPK=2 FakeLParens= FakeRParens=0 II=0x7fc8c980aa00 Text='delete2'
 M=0 C=0 T=OverloadedOperatorLParen S=0 B=0 BK=0 P=23 Name=l_paren L=22 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=void L=26 PPK=2 FakeLParens= FakeRParens=0 II=0x7fc8c9804fd0 Text='void'
 M=0 C=0 T=PointerOrReference S=0 B=0 BK=0 P=230 Name=star L=27 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='*'
 M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=240 Name=identifier L=31 PPK=2 FakeLParens= FakeRParens=0 II=0x7fc8c980aa30 Text='foo'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=43 Name=r_paren L=32 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=1 T=TrailingAnnotation S=1 B=0 BK=0 P=170 Name=identifier L=39 PPK=2 FakeLParens= FakeRParens=0 II=0x7fc8c980aa60 Text='ATTRIB'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=40 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

So for operator delete, clang-format decides that "operator" is a FunctionDeclarationName while it's an "Unknown" for delete2. The ) then becomes a CastRParen for delete but stays Unknown for delete2.

[nico]

Breadcrumbs:

isFunctionDeclarationName(), http://llvm-cs.pcc.me.uk/tools/clang/lib/Format/TokenAnnotator.cpp#1969

With this, both branches agree on the FunctionDeclarationName (but the cast and space are still different):

$ git diff
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index d032fd7cb95..20995770abf 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -1972,6 +1972,8 @@ static bool isFunctionDeclarationName(const FormatToken &Current,
return Next;
if (Next->is(TT_OverloadedOperator))
continue;

•  if (Next->is(tok::identifier))
  

•    continue;
   if (Next->isOneOf(tok::kw_new, tok::kw_delete)) {
     // For 'new[]' and 'delete[]'.
     if (Next->Next && Next->Next->is(tok::l_square) && Next->Next->Next &&
  

Nicos-MacBook-Pro:llvm-mono thakis$

[nico]

This fixes the bug:

diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index d032fd7cb95..4078faa604c 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -1490,8 +1490,7 @@ private:
// If there is an identifier (or with a few exceptions a keyword) right
// before the parentheses, this is unlikely to be a cast.
if (LeftOfParens->Tok.getIdentifierInfo() &&

•      !LeftOfParens->isOneOf(Keywords.kw_in, tok::kw_return, tok::kw_case,
  

•                             tok::kw_delete))
  

•      !LeftOfParens->isOneOf(Keywords.kw_in, tok::kw_return, tok::kw_case))
     return false;
  
   // Certain other tokens right before the parentheses are also signals that
  

It likely breaks formatting of expression-style delete, though. Probably need something like "if previous token is delete and the one before that is not operator".

[mydeveloperday]

Confirmed this is still an issue with clang-format v13

[mydeveloperday]

https://reviews.llvm.org/D116920