[clang-format] [PR45357] Fix issue found with operator spacing

[MyDeveloperDay]

Summary

This is a tentative fix for https://bugs.llvm.org/show_bug.cgi?id=45357

Spaces seem to be introduced between * and * due to changes brought in for D69573: [clang-format] [PR36294] AlwaysBreakAfterReturnType works incorrectly for some operator functions

We may need to gather some more use cases.

Pre the changes in D69573 the output was:

class UniquePtrGetterAddRefs {
  operator void **() { return reinterpret_cast<void **>(&mPtrStorage); }
}

class ReturnToGlobal {
  operator LocalRef<Object> *() { return &objRef; }
}

In the last windows snapshot (11.0.0.2263) (Feb 2020) the output was:

class UniquePtrGetterAddRefs {
  operator void * *() { return reinterpret_cast<void **>(&mPtrStorage); }
}

class ReturnToGlobal {
  operator LocalRef<Object> *() { return &objRef; }
}

post fix the out is:

class UniquePtrGetterAddRefs {
  operator void **() { return reinterpret_cast<void **>(&mPtrStorage); }
}

class ReturnToGlobal {
  operator LocalRef<Object>*() { return &objRef; }
}

Is the removal of the space between the <Object> and the* what you expect?

[MyDeveloperDay]

More test cases

[Abpostelnicu]

I think we are on the right track with this but what if we are dealing with tok::amp in a context similar to operator const FallibleTArray<E>&(); I can speculate that the outcome will be operator const FallibleTArray<E> &(); which is wrong.

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

2820

I'm not happy about this it feels way too general.

[MyDeveloperDay]

@sylvestre.ledru

I'm taking a quick look at formatting the original bug in gecko and whilst the last windows snapshot (Feb 2020) shows the bug

$ clang-format --version
clang-format version 11.0.0

nsTArray.h:939:29: warning: code should be clang-formatted [-Wclang-format-violations]
  operator const nsTArray<E>&() {
                            ^
nsTArray.h:946:35: warning: code should be clang-formatted [-Wclang-format-violations]
  operator const FallibleTArray<E>&() {
                                  ^
nsTArray.h:1147:59: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const nsTArray_Impl<E, Allocator>&() const& {
                                                          ^
nsTArray.h:1151:43: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const nsTArray<E>&() const& {
                                          ^
nsTArray.h:1154:49: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const FallibleTArray<E>&() const& {

The current trunk does not

$ clang-format --version
clang-format version 11.0.0 (https://github.com/llvm/llvm-project 1956a8a7cb79e94dbe073e36eba2d6b003f91046)

clang-format -n nsTArray.h

Is Mozilla toolchain using LLVM from the v10 branch?

I think this is fixed by D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107)

[Abpostelnicu]

In D78879#2004724, @MyDeveloperDay wrote:

@sylvestre.ledru

I'm taking a quick look at formatting the original bug in gecko and whilst the last windows snapshot (Feb 2020) shows the bug

$ clang-format --version
clang-format version 11.0.0

nsTArray.h:939:29: warning: code should be clang-formatted [-Wclang-format-violations]
  operator const nsTArray<E>&() {
                            ^
nsTArray.h:946:35: warning: code should be clang-formatted [-Wclang-format-violations]
  operator const FallibleTArray<E>&() {
                                  ^
nsTArray.h:1147:59: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const nsTArray_Impl<E, Allocator>&() const& {
                                                          ^
nsTArray.h:1151:43: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const nsTArray<E>&() const& {
                                          ^
nsTArray.h:1154:49: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const FallibleTArray<E>&() const& {

The current trunk does not

$ clang-format --version
clang-format version 11.0.0 (https://github.com/llvm/llvm-project 1956a8a7cb79e94dbe073e36eba2d6b003f91046)

clang-format -n nsTArray.h

Is Mozilla toolchain using LLVM from the v10 branch?

I think this is fixed by D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107)

Yes, we are using the clang tooling 10.

[MyDeveloperDay]

So for the Mozilla case mentioned in the bug report we either need to ask @hans if D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107) can go into the v10 branch, or perhaps they can apply that path rather than the revert patch

However I believe for @sylvestre.ledru bug ( is somewhat different and needs additional changes)

[Abpostelnicu]

I've cherry-picked D76850 to 10.x and see if this fixes the issue.

[MyDeveloperDay]

@sylvestre.ledru , @Abpostelnicu

I believe to fix your original request you need a combination of D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107) and this fix (this fix will handle the * * issue in gecko-dev/ipc/mscom/Ptr.h

I've run this new binary over gecko-dev/ipc/mscom and gecko-dev/xpcom/ds and it shows no clang-format warnings

I hope this helps

[MyDeveloperDay]

Need to skip over the template or we won't see this as needing to break on the return type

[hans]

In D78879#2004751, @MyDeveloperDay wrote:

So for the Mozilla case mentioned in the bug report we either need to ask @hans if D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107) can go into the v10 branch, or perhaps they can apply that path rather than the revert patch

Forwarding to @tstellar who manages the 10.0.1 release.

[Abpostelnicu]

In D78879#2004856, @MyDeveloperDay wrote:

@sylvestre.ledru , @Abpostelnicu

I believe to fix your original request you need a combination of D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107) and this fix (this fix will handle the * * issue in gecko-dev/ipc/mscom/Ptr.h

I've run this new binary over gecko-dev/ipc/mscom and gecko-dev/xpcom/ds and it shows no clang-format warnings

I hope this helps

Yes, definitely you are right D76850, only partially fixes the issue introduced by D69573, and your fix it's a good add-on in order to have a complete fix for the regression.

[Abpostelnicu]

Just to clarify something here, for me the patch looks good but until I will accept the revision I want to test it on the mozilla codebase.

clang/lib/Format/TokenAnnotator.cpp

for (; Next; Next = Next->Next) {
         continue;
       }
       if ((Next->isSimpleTypeSpecifier() || Next->is(tok::identifier)) &&
           Next->Next && Next->Next->isOneOf(tok::star, tok::amp, tok::ampamp)) {
         // For operator void*(), operator char*(), operator Foo*().
         Next = Next->Next;
         continue;
       }
+      if (Next->is(TT_TemplateOpener) && Next->MatchingParen) {
+        Next = Next->MatchingParen;
+        continue;
+      }
 
       break;
     }
     return nullptr;
   };
 
   // Find parentheses of parameter list.
   const FormatToken *Next = Current.Next;
return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
                            tok::l_paren) &&
             (Style.PointerAlignment != FormatStyle::PAS_Right &&
              !Line.IsMultiVariableDeclStmt) &&
             Left.Previous &&
             !Left.Previous->isOneOf(tok::l_paren, tok::coloncolon,
                                     tok::l_square));
   if (Right.is(tok::star) && Left.is(tok::l_paren))
     return false;
+  if (Right.is(tok::star) && Left.is(tok::star))
+    return false;
   if (Right.isOneOf(tok::star, tok::amp, tok::ampamp)) {
     const FormatToken *Previous = &Left;
     while (Previous && !Previous->is(tok::kw_operator)) {
       if (Previous->is(tok::identifier) || Previous->isSimpleTypeSpecifier()) {
         Previous = Previous->getPreviousNonComment();
         continue;
       }
       if (Previous->is(TT_TemplateCloser) && Previous->MatchingParen) {
         Previous = Previous->MatchingParen->getPreviousNonComment();
         continue;

clang/unittests/Format/FormatTest.cpp

verifyFormat("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",
+               "A::operator!() {}\n"
+               "void\n"
+               "A::operator**() {}\n"
+               "void\n"
+               "A::operator<Foo> *() {}\n"
+               "void\n"
+               "A::operator<Foo> **() {}\n"
+               "void\n"
+               "A::operator<Foo> &() {}\n"
+               "void\n"
+               "A::operator void **() {}\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 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"
verifyFormat("#if defined(while)\n"
                "#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*(void *);", Style);
   verifyFormat("Foo::operator*();", Style);
+  verifyFormat("Foo::operator**();", Style);
+  verifyFormat("Foo::operator&();", Style);
+  verifyFormat("Foo::operator<int> *();", Style);
+  verifyFormat("Foo::operator<Foo> *();", Style);
+  verifyFormat("Foo::operator<int> **();", Style);
+  verifyFormat("Foo::operator<Foo> **();", Style);
+  verifyFormat("Foo::operator<int> &();", Style);
+  verifyFormat("Foo::operator<Foo> &();", Style);
+  verifyFormat("Foo::operator<int> &&();", Style);
+  verifyFormat("Foo::operator<Foo> &&();", 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**();", 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("operator const nsTArrayRight<E> &()", Style);
+  verifyFormat("[[nodiscard]] operator const nsTArrayRight<E, Allocator> &()",
+               Style);
+  verifyFormat("operator void **()", Style);
+  verifyFormat("operator const FooRight<Object> &()", Style);
+  verifyFormat("operator const FooRight<Object> *()", Style);
+  verifyFormat("operator const FooRight<Object> **()", Style);
 
   Style.PointerAlignment = FormatStyle::PAS_Left;
   verifyFormat("Foo::operator*();", Style);
+  verifyFormat("Foo::operator**();", Style);
   verifyFormat("Foo::operator void*();", Style);
+  verifyFormat("Foo::operator void**();", Style);
   verifyFormat("Foo::operator/*comment*/ void*();", Style);
   verifyFormat("Foo::operator/*a*/ const /*b*/ void*();", Style);
   verifyFormat("Foo::operator/*a*/ volatile /*b*/ void*();", Style);
   verifyFormat("Foo::operator()(void*);", Style);
   verifyFormat("Foo::operator*(void*);", Style);
   verifyFormat("Foo::operator*();", Style);
+  verifyFormat("Foo::operator<int>*();", Style);
+  verifyFormat("Foo::operator<Foo>*();", Style);
+  verifyFormat("Foo::operator<int>**();", Style);
+  verifyFormat("Foo::operator<Foo>**();", Style);
+  verifyFormat("Foo::operator<int>&();", Style);
+  verifyFormat("Foo::operator<Foo>&();", Style);
+  verifyFormat("Foo::operator<int>&&();", Style);
+  verifyFormat("Foo::operator<Foo>&&();", Style);
   verifyFormat("operator*(int (*)(), class Foo);", Style);
 
   verifyFormat("Foo::operator&();", Style);
   verifyFormat("Foo::operator void&();", Style);
   verifyFormat("Foo::operator/*comment*/ void&();", Style);
   verifyFormat("Foo::operator/*a*/ const /*b*/ void&();", Style);
   verifyFormat("Foo::operator/*a*/ volatile /*b*/ 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/*comment*/ void&&();", Style);
   verifyFormat("Foo::operator/*a*/ const /*b*/ void&&();", Style);
   verifyFormat("Foo::operator/*a*/ volatile /*b*/ void&&();", Style);
   verifyFormat("Foo::operator()(void&&);", Style);
   verifyFormat("Foo::operator&&(void&&);", Style);
   verifyFormat("Foo::operator&&();", Style);
   verifyFormat("operator&&(int(&&)(), class Foo);", Style);
+  verifyFormat("operator const nsTArrayLeft<E>&()", Style);
+  verifyFormat("[[nodiscard]] operator const nsTArrayLeft<E, Allocator>&()",
+               Style);
+  verifyFormat("operator void**()", Style);
+  verifyFormat("operator const FooLeft<Object>&()", Style);
+  verifyFormat("operator const FooLeft<Object>*()", Style);
+  verifyFormat("operator const FooLeft<Object>**()", Style);
 
   // PR45107
   verifyFormat("operator Vector<String>&();", Style);
+  verifyFormat("operator const Vector<String>&();", Style);
   verifyFormat("operator foo::Bar*();", Style);
   verifyFormat("operator const Foo<X>::Bar<Y>*();", Style);
   verifyFormat("operator/*a*/ const /*b*/ Foo /*c*/<X> /*d*/ ::Bar<Y>*();",
                Style);
 
   Style.PointerAlignment = FormatStyle::PAS_Middle;
   verifyFormat("Foo::operator*();", Style);
   verifyFormat("Foo::operator void *();", Style);