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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Let me look into this a little, I'd prefer we fixed the corner cases than 
reverted the lot (https://bugs.llvm.org/show_bug.cgi?id=45357)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

FYI, in Mozilla build of clang-format, we reverted this change. 
It was causing more issues than fixes.
https://bugzilla.mozilla.org/show_bug.cgi?id=1629853


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

@hans done here: 
https://github.com/llvm/llvm-project/commit/50eedc134a219ef6d2345e4efc5471a2e3824223
 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D69573#1900089 , @sylvestre.ledru 
wrote:

> @MyDeveloperDay @hans what about adding this to the release notes?
>  I was trying clang-format 10 on Firefox code base and I noticed this change 
> which isn't documented in
>  
> https://prereleases.llvm.org/10.0.0/rc1/tools/clang/docs/ReleaseNotes.html#clang-format
>  (I can do it if you want)


Release notes are very welcome :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added subscribers: hans, sylvestre.ledru.
sylvestre.ledru added a comment.

@MyDeveloperDay @hans what about adding this to the release notes?
I was trying clang-format 10 on Firefox code base and I noticed this change 
which isn't documented in
https://prereleases.llvm.org/10.0.0/rc1/tools/clang/docs/ReleaseNotes.html#clang-format
(I can do it if you want)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa75f8d98d7ac: [clang-format] [PR36294] 
AlwaysBreakAfterReturnType works incorrectly for some… (authored by 
MyDeveloperDay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69573

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6134,7 +6134,48 @@
"void\n"
"A::operator>>() {}\n"
"void\n"
-   "A::operator+() {}\n",
+   "A::operator+() {}\n"
+   "void\n"
+   "A::operator*() {}\n"
+   "void\n"
+   "A::operator->() {}\n"
+   "void\n"
+   "A::operator void *() {}\n"
+   "void\n"
+   "A::operator void &() {}\n"
+   "void\n"
+   "A::operator void &&() {}\n"
+   "void\n"
+   "A::operator char *() {}\n"
+   "void\n"
+   "A::operator[]() {}\n"
+   "void\n"
+   "A::operator!() {}\n",
+   Style);
+  verifyFormat("constexpr auto\n"
+   "operator()() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator>>() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator+() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator*() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator->() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator++() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator void *() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator void &() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator void &&() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator char *() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator!() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator[]() const -> reference {}\n",
Style);
   verifyFormat("void *operator new(std::size_t s);", // No break here.
Style);
@@ -14755,6 +14796,53 @@
"#endif // while");
 }
 
+TEST_F(FormatTest, OperatorSpacing) {
+  FormatStyle Style = getLLVMStyle();
+  Style.PointerAlignment = FormatStyle::PAS_Right;
+  verifyFormat("Foo::operator*();", Style);
+  verifyFormat("Foo::operator void *();", Style);
+  verifyFormat("Foo::operator()(void *);", Style);
+  verifyFormat("Foo::operator*(void *);", Style);
+  verifyFormat("Foo::operator*();", Style);
+  verifyFormat("operator*(int (*)(), class Foo);", Style);
+
+  verifyFormat("Foo::operator&();", Style);
+  verifyFormat("Foo::operator void &();", Style);
+  verifyFormat("Foo::operator()(void &);", Style);
+  verifyFormat("Foo::operator&(void &);", Style);
+  verifyFormat("Foo::operator&();", Style);
+  verifyFormat("operator&(int (&)(), class Foo);", Style);
+
+  verifyFormat("Foo::operator&&();", Style);
+  verifyFormat("Foo::operator void &&();", Style);
+  verifyFormat("Foo::operator()(void &&);", Style);
+  verifyFormat("Foo::operator&&(void &&);", Style);
+  verifyFormat("Foo::operator&&();", Style);
+  verifyFormat("operator&&(int(&&)(), class Foo);", Style);
+
+  Style.PointerAlignment = FormatStyle::PAS_Left;
+  verifyFormat("Foo::operator*();", Style);
+  verifyFormat("Foo::operator void*();", Style);
+  verifyFormat("Foo::operator()(void*);", Style);
+  verifyFormat("Foo::operator*(void*);", Style);
+  verifyFormat("Foo::operator*();", Style);
+  verifyFormat("operator*(int (*)(), class Foo);", Style);
+
+  verifyFormat("Foo::operator&();", Style);
+  verifyFormat("Foo::operator void&();", Style);
+  verifyFormat("Foo::operator()(void&);", Style);
+  verifyFormat("Foo::operator&(void&);", Style);
+  verifyFormat("Foo::operator&();", Style);
+  verifyFormat("operator&(int (&)(), class Foo);", Style);
+
+  verifyFormat("Foo::operator&&();", Style);
+  verifyFormat("Foo::operator void&&();", Style);
+  verifyFormat("Foo::operator()(void&&);", Style);
+  verifyFormat("Foo::operator&&(void&&);", Style);
+  verifyFormat("Foo::operator&&();", Style);
+  verifyFormat("operator&&(int(&&)(), class Foo);", Style);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -950,9 +950,9 @@
 if (CurrentToken->isOn

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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks, looks great!
Sorry for the delay, I was out last week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a comment.

gentle ping!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Rian Quinn via Phabricator via cfe-commits]

rianquinn added a comment.

Looking at the unit tests, it appears that you have successfully fixed the bug


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 59827 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 227405.
MyDeveloperDay set the repository for this revision to rG LLVM Github Monorepo.
MyDeveloperDay added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69573

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6111,7 +6111,48 @@
"void\n"
"A::operator>>() {}\n"
"void\n"
-   "A::operator+() {}\n",
+   "A::operator+() {}\n"
+   "void\n"
+   "A::operator*() {}\n"
+   "void\n"
+   "A::operator->() {}\n"
+   "void\n"
+   "A::operator void *() {}\n"
+   "void\n"
+   "A::operator void &() {}\n"
+   "void\n"
+   "A::operator void &&() {}\n"
+   "void\n"
+   "A::operator char *() {}\n"
+   "void\n"
+   "A::operator[]() {}\n"
+   "void\n"
+   "A::operator!() {}\n",
+   Style);
+  verifyFormat("constexpr auto\n"
+   "operator()() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator>>() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator+() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator*() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator->() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator++() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator void *() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator void &() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator void &&() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator char *() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator!() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator[]() const -> reference {}\n",
Style);
   verifyFormat("void *operator new(std::size_t s);", // No break here.
Style);
@@ -14728,6 +14769,53 @@
"#endif // while");
 }
 
+TEST_F(FormatTest, OperatorSpacing) {
+  FormatStyle Style = getLLVMStyle();
+  Style.PointerAlignment = FormatStyle::PAS_Right;
+  verifyFormat("Foo::operator*();", Style);
+  verifyFormat("Foo::operator void *();", Style);
+  verifyFormat("Foo::operator()(void *);", Style);
+  verifyFormat("Foo::operator*(void *);", Style);
+  verifyFormat("Foo::operator*();", Style);
+  verifyFormat("operator*(int (*)(), class Foo);", Style);
+
+  verifyFormat("Foo::operator&();", Style);
+  verifyFormat("Foo::operator void &();", Style);
+  verifyFormat("Foo::operator()(void &);", Style);
+  verifyFormat("Foo::operator&(void &);", Style);
+  verifyFormat("Foo::operator&();", Style);
+  verifyFormat("operator&(int (&)(), class Foo);", Style);
+
+  verifyFormat("Foo::operator&&();", Style);
+  verifyFormat("Foo::operator void &&();", Style);
+  verifyFormat("Foo::operator()(void &&);", Style);
+  verifyFormat("Foo::operator&&(void &&);", Style);
+  verifyFormat("Foo::operator&&();", Style);
+  verifyFormat("operator&&(int(&&)(), class Foo);", Style);
+
+  Style.PointerAlignment = FormatStyle::PAS_Left;
+  verifyFormat("Foo::operator*();", Style);
+  verifyFormat("Foo::operator void*();", Style);
+  verifyFormat("Foo::operator()(void*);", Style);
+  verifyFormat("Foo::operator*(void*);", Style);
+  verifyFormat("Foo::operator*();", Style);
+  verifyFormat("operator*(int (*)(), class Foo);", Style);
+
+  verifyFormat("Foo::operator&();", Style);
+  verifyFormat("Foo::operator void&();", Style);
+  verifyFormat("Foo::operator()(void&);", Style);
+  verifyFormat("Foo::operator&(void&);", Style);
+  verifyFormat("Foo::operator&();", Style);
+  verifyFormat("operator&(int (&)(), class Foo);", Style);
+
+  verifyFormat("Foo::operator&&();", Style);
+  verifyFormat("Foo::operator void&&();", Style);
+  verifyFormat("Foo::operator()(void&&);", Style);
+  verifyFormat("Foo::operator&&(void&&);", Style);
+  verifyFormat("Foo::operator&&();", Style);
+  verifyFormat("operator&&(int(&&)(), class Foo);", Style);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -950,9 +950,9 @@
 if (CurrentToken->isOneOf(tok::star, tok::amp))
  

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 5 inline comments as done.
MyDeveloperDay added inline comments.



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*().

sammccall wrote:
> I'm suspicious of code that handles star but not amp, though maybe I 
> shouldn't be.
added those and mutated your test cases to cover that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 227109.
MyDeveloperDay added a comment.

Remove test regression


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

https://reviews.llvm.org/D69573

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6111,7 +6111,40 @@
"void\n"
"A::operator>>() {}\n"
"void\n"
-   "A::operator+() {}\n",
+   "A::operator+() {}\n"
+   "void\n"
+   "A::operator*() {}\n"
+   "void\n"
+   "A::operator->() {}\n"
+   "void\n"
+   "A::operator void *() {}\n"
+   "void\n"
+   "A::operator char *() {}\n"
+   "void\n"
+   "A::operator[]() {}\n"
+   "void\n"
+   "A::operator!() {}\n",
+   Style);
+  verifyFormat("constexpr auto\n"
+   "operator()() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator>>() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator+() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator*() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator->() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator++() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator void *() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator char *() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator!() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator[]() const -> reference {}\n",
Style);
   verifyFormat("void *operator new(std::size_t s);", // No break here.
Style);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -952,7 +952,7 @@
 consumeToken();
 if (CurrentToken &&
 CurrentToken->Previous->isOneOf(TT_BinaryOperator, TT_UnaryOperator,
-tok::comma))
+tok::comma, tok::star, tok::arrow))
   CurrentToken->Previous->Type = TT_OverloadedOperator;
   }
   if (CurrentToken) {
@@ -1344,8 +1344,12 @@
 Contexts.back().IsExpression = false;
 } else if (Current.is(tok::kw_new)) {
   Contexts.back().CanBeExpression = false;
-} else if (Current.isOneOf(tok::semi, tok::exclaim)) {
+} else if (Current.is(tok::semi) ||
+   (Current.is(tok::exclaim) && Current.Previous &&
+!Current.Previous->is(tok::kw_operator))) {
   // This should be the condition or increment in a for-loop.
+  // But not operator !() (can't use TT_OverloadedOperator here as its not
+  // been annotated yet).
   Contexts.back().IsExpression = true;
 }
   }
@@ -2087,11 +2091,22 @@
 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 &&
-Next->Next->Next->is(tok::r_square))
+if (Next->Next &&
+Next->Next->startsSequence(tok::l_square, tok::r_square))
   Next = Next->Next->Next;
 continue;
   }
+  if (Next->startsSequence(tok::l_square, tok::r_square)) {
+// For operator[]().
+Next = Next->Next;
+continue;
+  }
+  if ((Next->isSimpleTypeSpecifier() || Next->is(tok::identifier)) &&
+  Next->Next && Next->Next->is(tok::star)) {
+// For operator void*(), operator char*(), operator Foo*().
+Next = Next->Next;
+continue;
+  }
 
   break;
 }
@@ -2605,6 +2620,12 @@
 tok::l_square));
   if (Right.is(tok::star) && Left.is(tok::l_paren))
 return false;
+  if (Right.is(tok::star) &&
+  (Left.is(tok::identifier) || Left.isSimpleTypeSpecifier()) &&
+  Left.Previous && Left.Previous->is(tok::kw_operator))
+// No space between the type and the *
+// operator void*(), operator char*(), operator Foo*().
+return (Style.PointerAlignment == FormatStyle::PAS_Right);
   const auto SpaceRequiredForArrayInitializerLSquare =
   [](const FormatToken &LSquareTok, const FormatStyle &Style) {
 return Style.SpacesInContainerLiterals ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/c

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 4 inline comments as done.
MyDeveloperDay added inline comments.



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*().

sammccall wrote:
> why?
LLVM style and google style is different based on pointer alignment



Comment at: clang/unittests/Format/FormatTest.cpp:6989
   verifyFormat("operator int();");
-  verifyFormat("operator void *();");
+  verifyFormat("operator void*();");
   verifyFormat("operator SomeType();");

MyDeveloperDay wrote:
> 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.
reverted


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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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();");

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();");

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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;

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?



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*().

why?



Comment at: clang/unittests/Format/FormatTest.cpp:6989
   verifyFormat("operator int();");
-  verifyFormat("operator void *();");
+  verifyFormat("operator void*();");
   verifyFormat("operator SomeType();");

this looks like a regression at first glance, in LLVM style there's a space 
between type and *


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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 227083.
MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added a comment.

Extend this revision to cover additional 
https://bugs.llvm.org/show_bug.cgi?id=43783 issue (which has overlap)

New revision correctly formats the following code:

  constexpr auto
  operator*() const -> reference {
bfexpects_when_compliant(m_i < m_t->size());
return m_t->get()[m_i];
  }
  
  constexpr auto
  operator->() const -> pointer {
bfexpects_when_compliant(m_i < m_t->size());
return &m_t->get()[m_i];
  }
  
  constexpr auto
  operator[](index_type n) const -> reference {
bfexpects_when_compliant(m_i < m_t->size());
return m_t->get()[m_i];
  }
  
  constexpr auto
  operator++() -> lra_iterator & {
++m_i;
return *this;
  }
  
  constexpr auto
  operator()() const -> reference {}
  
  constexpr auto
  operator++() const -> reference {}
  
  constexpr auto
  operator--() const -> reference {}
  
  constexpr auto
  operator*() const -> reference {}
  
  constexpr auto
  operator->() const -> reference {}
  
  constexpr auto
  operator>>() const -> reference {}
  
  constexpr auto
  operator<<() const -> reference {}
  
  constexpr auto
  operator[]() const -> reference {}
  
  constexpr auto
  operator void*() const -> reference {}
  
  constexpr auto
  operator char*() const -> reference {}
  
  constexpr auto
  operator Foo*() const -> reference {}
  
  constexpr auto
  operator!() const -> reference {}
  
  class Foo {
  public:
bool operator!() const;
bool operator<(Foo const &) const;
bool operator*() const;
bool operator->() const;
bool operator+() const;
bool operator-() const;
bool f() const;
  };
  
  bool
  Foo::operator!() const {
return true;
  }
  bool
  Foo::operator<(Foo const &) const {
return true;
  }
  bool
  Foo::operator*() const {
return true;
  }
  bool
  Foo::operator->() const {
return true;
  }
  bool
  Foo::operator+() const {
return true;
  }
  bool
  Foo::operator-() const {
return true;
  }
  bool
  Foo::f() const {
return true;
  }


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

https://reviews.llvm.org/D69573

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6111,7 +6111,40 @@
"void\n"
"A::operator>>() {}\n"
"void\n"
-   "A::operator+() {}\n",
+   "A::operator+() {}\n"
+   "void\n"
+   "A::operator*() {}\n"
+   "void\n"
+   "A::operator->() {}\n"
+   "void\n"
+   "A::operator void*() {}\n"
+   "void\n"
+   "A::operator char*() {}\n"
+   "void\n"
+   "A::operator[]() {}\n"
+   "void\n"
+   "A::operator!() {}\n",
+   Style);
+  verifyFormat("constexpr auto\n"
+   "operator()() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator>>() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator+() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator*() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator->() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator++() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator void*() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator char*() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator!() const -> reference {}\n"
+   "constexpr auto\n"
+   "operator[]() const -> reference {}\n",
Style);
   verifyFormat("void *operator new(std::size_t s);", // No break here.
Style);
@@ -6953,7 +6986,7 @@
   verifyFormat("bool operator[]();");
   verifyFormat("operator bool();");
   verifyFormat("operator int();");
-  verifyFormat("operator void *();");
+  verifyFormat("operator void*();");
   verifyFormat("operator SomeType();");
   verifyFormat("operator SomeType();");
   verifyFormat("operator SomeType>();");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -952,7 +952,7 @@
 consumeToken();
 if (CurrentToken &&
 CurrentToken->Previous->isOneOf(TT_BinaryOperator, TT_UnaryOperator,
-tok::comma))
+tok::comma, tok::star, tok::arrow))
   CurrentToken->Previous->Type =

[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:2090
 continue;
+  if (Next->isOneOf(tok::star, tok::arrow))
+continue;

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?


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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 4 inline comments as done.
MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:1347
   Contexts.back().CanBeExpression = false;
-} else if (Current.isOneOf(tok::semi, tok::exclaim)) {
+} else if (Current.isOneOf(tok::semi, tok::exclaim) &&
+   !(Current.Previous && Current.Previous->is(tok::kw_operator))) {

sammccall wrote:
> This seems clearer as `is semi || (is exclaim && !previous is operator)`, as 
> `operator;` isn't relevant here.
> `&& !Current.is(TT_OverloadedOperator) would be even better of course`
rearranged the logic as suggested, unfortunately we can't use 
TT_LoverloadOperator at this point because its not yet been annotated (so ! has 
TT_Unknown still)



Comment at: clang/lib/Format/TokenAnnotator.cpp:2090
 continue;
+  if (Next->isOneOf(tok::star, tok::arrow))
+continue;

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.


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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 226961.
MyDeveloperDay added a comment.

Trying to address review comments


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

https://reviews.llvm.org/D69573

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6111,7 +6111,13 @@
"void\n"
"A::operator>>() {}\n"
"void\n"
-   "A::operator+() {}\n",
+   "A::operator+() {}\n"
+   "void\n"
+   "A::operator*() {}\n"
+   "void\n"
+   "A::operator->() {}\n"
+   "void\n"
+   "A::operator!() {}\n",
Style);
   verifyFormat("void *operator new(std::size_t s);", // No break here.
Style);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -952,7 +952,7 @@
 consumeToken();
 if (CurrentToken &&
 CurrentToken->Previous->isOneOf(TT_BinaryOperator, 
TT_UnaryOperator,
-tok::comma))
+tok::comma, tok::arrow))
   CurrentToken->Previous->Type = TT_OverloadedOperator;
   }
   if (CurrentToken) {
@@ -1344,8 +1344,12 @@
 Contexts.back().IsExpression = false;
 } else if (Current.is(tok::kw_new)) {
   Contexts.back().CanBeExpression = false;
-} else if (Current.isOneOf(tok::semi, tok::exclaim)) {
+} else if (Current.is(tok::semi) ||
+   (Current.is(tok::exclaim) && Current.Previous &&
+!Current.Previous->is(tok::kw_operator))) {
   // This should be the condition or increment in a for-loop.
+  // But not operator !() (can't use TT_OverloadedOperator here as its not
+  // been annotated yet).
   Contexts.back().IsExpression = true;
 }
   }
@@ -2085,6 +2089,8 @@
 return Next;
   if (Next->is(TT_OverloadedOperator))
 continue;
+  if (Next->is(TT_PointerOrReference))
+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 &&


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6111,7 +6111,13 @@
"void\n"
"A::operator>>() {}\n"
"void\n"
-   "A::operator+() {}\n",
+   "A::operator+() {}\n"
+   "void\n"
+   "A::operator*() {}\n"
+   "void\n"
+   "A::operator->() {}\n"
+   "void\n"
+   "A::operator!() {}\n",
Style);
   verifyFormat("void *operator new(std::size_t s);", // No break here.
Style);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -952,7 +952,7 @@
 consumeToken();
 if (CurrentToken &&
 CurrentToken->Previous->isOneOf(TT_BinaryOperator, TT_UnaryOperator,
-tok::comma))
+tok::comma, tok::arrow))
   CurrentToken->Previous->Type = TT_OverloadedOperator;
   }
   if (CurrentToken) {
@@ -1344,8 +1344,12 @@
 Contexts.back().IsExpression = false;
 } else if (Current.is(tok::kw_new)) {
   Contexts.back().CanBeExpression = false;
-} else if (Current.isOneOf(tok::semi, tok::exclaim)) {
+} else if (Current.is(tok::semi) ||
+   (Current.is(tok::exclaim) && Current.Previous &&
+!Current.Previous->is(tok::kw_operator))) {
   // This should be the condition or increment in a for-loop.
+  // But not operator !() (can't use TT_OverloadedOperator here as its not
+  // been annotated yet).
   Contexts.back().IsExpression = true;
 }
   }
@@ -2085,6 +2089,8 @@
 return Next;
   if (Next->is(TT_OverloadedOperator))
 continue;
+  if (Next->is(TT_PointerOrReference))
+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 &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:1347
   Contexts.back().CanBeExpression = false;
-} else if (Current.isOneOf(tok::semi, tok::exclaim)) {
+} else if (Current.isOneOf(tok::semi, tok::exclaim) &&
+   !(Current.Previous && Current.Previous->is(tok::kw_operator))) {

This seems clearer as `is semi || (is exclaim && !previous is operator)`, as 
`operator;` isn't relevant here.
`&& !Current.is(TT_OverloadedOperator) would be even better of course`



Comment at: clang/lib/Format/TokenAnnotator.cpp:2090
 continue;
+  if (Next->isOneOf(tok::star, tok::arrow))
+continue;

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: mitchell-stellar, klimek, owenpan, sammccall.
MyDeveloperDay added projects: clang-format, clang-tools-extra.
Herald added a project: clang.

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

Addressing bug related to returning after return type not being honoured for 
some operator types.

  $ bin/clang-format --style="{BasedOnStyle: llvm, AlwaysBreakAfterReturnType: 
TopLevelDefinitions}" /tmp/foo.cpp
  class Foo {
  public:
bool operator!() const;
bool operator<(Foo const &) const;
bool operator*() const;
bool operator->() const;
bool operator+() const;
bool operator-() const;
bool f() const;
  };
  
  bool Foo::operator!() const { return true; }
  bool
  Foo::operator<(Foo const &) const {
return true;
  }
  bool Foo::operator*() const { return true; }
  bool Foo::operator->() const { return true; }
  bool
  Foo::operator+() const {
return true;
  }
  bool
  Foo::operator-() const {
return true;
  }
  bool
  Foo::f() const {
return true;
  }


Repository:
  rC Clang

https://reviews.llvm.org/D69573

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6111,7 +6111,13 @@
"void\n"
"A::operator>>() {}\n"
"void\n"
-   "A::operator+() {}\n",
+   "A::operator+() {}\n"
+   "void\n"
+   "A::operator*() {}\n"
+   "void\n"
+   "A::operator->() {}\n"
+   "void\n"
+   "A::operator!() {}\n",
Style);
   verifyFormat("void *operator new(std::size_t s);", // No break here.
Style);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1344,8 +1344,10 @@
 Contexts.back().IsExpression = false;
 } else if (Current.is(tok::kw_new)) {
   Contexts.back().CanBeExpression = false;
-} else if (Current.isOneOf(tok::semi, tok::exclaim)) {
+} else if (Current.isOneOf(tok::semi, tok::exclaim) &&
+   !(Current.Previous && Current.Previous->is(tok::kw_operator))) {
   // This should be the condition or increment in a for-loop.
+  // but not operator !()
   Contexts.back().IsExpression = true;
 }
   }
@@ -2085,6 +2087,8 @@
 return Next;
   if (Next->is(TT_OverloadedOperator))
 continue;
+  if (Next->isOneOf(tok::star, tok::arrow))
+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 &&


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6111,7 +6111,13 @@
"void\n"
"A::operator>>() {}\n"
"void\n"
-   "A::operator+() {}\n",
+   "A::operator+() {}\n"
+   "void\n"
+   "A::operator*() {}\n"
+   "void\n"
+   "A::operator->() {}\n"
+   "void\n"
+   "A::operator!() {}\n",
Style);
   verifyFormat("void *operator new(std::size_t s);", // No break here.
Style);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1344,8 +1344,10 @@
 Contexts.back().IsExpression = false;
 } else if (Current.is(tok::kw_new)) {
   Contexts.back().CanBeExpression = false;
-} else if (Current.isOneOf(tok::semi, tok::exclaim)) {
+} else if (Current.isOneOf(tok::semi, tok::exclaim) &&
+   !(Current.Previous && Current.Previous->is(tok::kw_operator))) {
   // This should be the condition or increment in a for-loop.
+  // but not operator !()
   Contexts.back().IsExpression = true;
 }
   }
@@ -2085,6 +2087,8 @@
 return Next;
   if (Next->is(TT_OverloadedOperator))
 continue;
+  if (Next->isOneOf(tok::star, tok::arrow))
+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 &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits