[PATCH] D69573: [clang-format] [PR36294] AlwaysBreakAfterReturnType works incorrectly for some operator functions

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:955
             CurrentToken->Previous->isOneOf(TT_BinaryOperator, TT_UnaryOperator,
-                                            tok::comma))
+                                            tok::comma, tok::star, tok::arrow))
           CurrentToken->Previous->Type = TT_OverloadedOperator;
----------------
MyDeveloperDay wrote:
> MyDeveloperDay wrote:
> > sammccall wrote:
> > > I'm confused about this: ISTM that where we were previously always treating star as a pointer, now we're always treating it as an operator name.
> > > Whereas it's sometimes an operator name (immediately after the `operator` keyword) and sometimes a pointer (following a type name).
> > > 
> > > Maybe we should shift the OverloadedOperator labelling outside the loop (since AFAICT it should only apply to the first token) and then keep the loop to mark stars/amps elsewhere in the operator name as PointerOrReference?
> > Let me take another look..
> @sammccall  In trying to understand what you were suggesting I tried the following:
> 
> ```
> bool
> Foo::operator*(void *) const {
>   return true;
> }
> ```
> 
> The second `*` will be as seen correctly below a PointerOrReference by the nature that we already hit the OverloadedOperatorLParen as the loop only goes until it see a `(` `)` or `;`
> 
> I'm trying to think of a case where that perhaps wouldn't be the case? such that we might get 2 OverloadedOperators
> 
> ```
> M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=bool L=4 PPK=2 FakeLParens= FakeRParens=0 II=0x1cea7c36c40 Text='bool'
>  M=1 C=1 T=FunctionDeclarationName S=1 B=0 BK=0 P=80 Name=identifier L=84 PPK=2 FakeLParens= FakeRParens=0 II=0x1cea7c42298 Text='Foo'
>  M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=coloncolon L=86 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='::'
>  M=0 C=1 T=Unknown S=0 B=0 BK=0 P=520 Name=operator L=94 PPK=2 FakeLParens= FakeRParens=0 II=0x1cea7c36ed8 Text='operator'
>  M=0 C=0 T=OverloadedOperator S=0 B=0 BK=0 P=34 Name=star L=95 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='*'
>  M=0 C=0 T=OverloadedOperatorLParen S=0 B=0 BK=0 P=34 Name=l_paren L=96 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=100 PPK=2 FakeLParens= FakeRParens=0 II=0x1cea7c368e0 Text='void'
>  M=0 C=1 T=PointerOrReference S=1 B=0 BK=0 P=230 Name=star L=102 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='*'
>  M=0 C=0 T=Unknown S=0 B=0 BK=0 P=54 Name=r_paren L=103 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
>  M=0 C=1 T=TrailingAnnotation S=1 B=0 BK=0 P=170 Name=const L=109 PPK=2 FakeLParens= FakeRParens=0 II=0x1cea7c363e8 Text='const'
>  M=0 C=0 T=FunctionLBrace S=1 B=0 BK=1 P=23 Name=l_brace L=111 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='{'
> ```
I mean something like a conversion operator to pointer: `bool Foo::operator void *();`
The star appears before the operator arg list, but it's still a pointer.

So the testcases I'd try here would be:

```
Foo::operator*(); // should be OverloadedOperator
Foo::operator void *(); // should be PointerOrReference
Foo::operator()(void *); // should be PointerOrReference
Foo::operator*(void *); // should be OverloadedOperator, PointerOrReference
operator*(int(*)(), class Foo); // OverloadedOperator, PointerOrReference (this one is silly and doesn't need to work)
```


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2105
+      if ((Next->isSimpleTypeSpecifier() || Next->is(tok::identifier)) &&
+          Next->Next && Next->Next->is(tok::star)) {
+        // For operator void*(), operator char*(), operator Foo*().
----------------
I'm suspicious of code that handles star but not amp, though maybe I shouldn't be.


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

https://reviews.llvm.org/D69573