[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:2090
         continue;
+      if (Next->isOneOf(tok::star, tok::arrow))
+        continue;
----------------
sammccall wrote:
> MyDeveloperDay wrote:
> > sammccall wrote:
> > > This seems like it's supposed to be handled by the token annotator, and the same cause will result in bugs in other places - why aren't these tokens being marked as TT_OverloadedOperator?
> > > 
> > > I'd guess the deeper fix is in the `kw_operator` case in consumeToken(). The way it's written looks suspect, though I'm not an expert on any of this.
> > The * from *() is already labelled as a TT_PointerOrRefernce so can't also be marked as an Overloaded operator so use that in the loop.
> Right, the fact that it's incorrectly labeled as a PointerOrReference is a bug, is it not possible to fix that instead of working around it here?
> 
> In the consumeToken loop, it seems the `*` in `operator *`  is labeled as PointerOrReference by logic that's trying to handle e.g. `StringRef::operator char*()`. However when the `*` is _immediately_ preceded by `operator`, it's always an overloaded operator name rather than a pointer, I think?
Treating `*` and `->` as an TT_OverloadedOperator is probably ok, although this breaks unit test I highlight below, which I almost think is incorrect anyway.

I'm looking into a related issue https://bugs.llvm.org/show_bug.cgi?id=43783, which is changing this code again, I may roll both changes into the same fix

However this is made much more complex to deal with it just by using the `TT_OverloadedOperator` when the operator consists of 2 tokens say like `[` and `]` unless I combine them into a single token.

```
operator void*()
operator char*()
operator []()
```

hence the need for code to handle in this loop for handling.

```
operator new[]()
operator delete[]()
```


================
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;
----------------
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..


================
Comment at: clang/unittests/Format/FormatTest.cpp:6989
   verifyFormat("operator int();");
-  verifyFormat("operator void *();");
+  verifyFormat("operator void*();");
   verifyFormat("operator SomeType<int>();");
----------------
sammccall wrote:
> this looks like a regression at first glance, in LLVM style there's a space between type and *
yes, I was a little concerned about this change too, it's hard to know if `operator void *` was intended as it falls off the bottom of the spacesBetween function with a return true; but if you are concerned then perhaps its better to change it back.


================
Comment at: clang/unittests/Format/FormatTest.cpp:6962
   verifyFormat("operator int();");
   verifyFormat("operator void *();");
   verifyFormat("operator SomeType<int>();");
----------------
Treating * as TT_OverloadedOperator will break this test, which perhaps is already wrong!


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

https://reviews.llvm.org/D69573