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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay 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:
> 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='{'
```


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2626
+      Left.Previous && Left.Previous->is(tok::kw_operator))
+    // No space between the type and the *
+    // operator void*(), operator char*(), operator Foo*().
----------------
MyDeveloperDay wrote:
> sammccall wrote:
> > why?
> LLVM style and google style is different based on pointer alignment
I believe that now I'm no longer marking that `*` as a TT_PointerOrReference, one of the other `spaceRequiredBetween` rules is missing for this case and now we need to consider the pointer alignment.

Without adding this rule to handle this case one of the 2 items from the same `UnderstandsOverloadedOperators` test fail

```
verifyFormat("operator void *();");
...
verifyGoogleFormat("operator void*();");
```

its possible I could clarify this further make ensuring the star is also an overloaded operator e.g.

```
if (Right.is(tok::star)  && Right.is(TT_OverloadedOperator)
```


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

https://reviews.llvm.org/D69573