[clang-format] [PR46157] Wrong spacing of negative literals with use of operator

[MyDeveloperDay]

Summary

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

class A;

void foo(A (*)(const A&, const A&), int);
A operator+(const A&, const A&);

void bar() {
  return foo(operator+, -42);
}

is incorrect formatted putting a space between the - and the 42

return foo(operator+, - 42);

This revision tries to refine the annotating of the OverloadOperator to only mark the comma as an operator if its immediately after the operator keyword and to not mark the closing '(' as an TT_OverloadedOperatorLParen unless its actually a (

This code was assuming the usage of operator too much I suspect and not considering its use as a function ptr.

Probably a fairly rare corner case.

[thopre]

That was a speedy patch! I can confirm that my non reduced testcase (git diff -U0 23ac16cf9bd4cc0bb434efcf6385baf083a2ff7b^ 23ac16cf9bd4cc0bb434efcf6385baf083a2ff7b | clang-format-diff.py -i -p1) is fixed with this patch. Thanks! I don't feel confident reviewing the code unfortunately.

[MyDeveloperDay]

In D80933#2066648, @thopre wrote:

That was a speedy patch! I can confirm that my non reduced testcase (git diff -U0 23ac16cf9bd4cc0bb434efcf6385baf083a2ff7b^ 23ac16cf9bd4cc0bb434efcf6385baf083a2ff7b | clang-format-diff.py -i -p1) is fixed with this patch. Thanks! I don't feel confident reviewing the code unfortunately.

No problems, we can wait for the others.

[curdeius]

clang/lib/Format/TokenAnnotator.cpp

981

Shouldn't you check CurrentToken for not being null after a call to consumeToken?? It may happen if operator is at the end of input (even if it's a degenerated case).

[MyDeveloperDay]

Ensure no crash when CurrentToken is nullptr

[curdeius]

LGTM.
Keep in mind I'm not the code owner, I don't know if another approval is required.

clang/lib/Format/TokenAnnotator.cpp

case tok::kw_operator:
       if (Style.Language == FormatStyle::LK_TextProto ||
           Style.Language == FormatStyle::LK_Proto)
         break;
       while (CurrentToken &&
              !CurrentToken->isOneOf(tok::l_paren, tok::semi, tok::r_paren)) {
         if (CurrentToken->isOneOf(tok::star, tok::amp))
           CurrentToken->setType(TT_PointerOrReference);
         consumeToken();
+        if (CurrentToken && CurrentToken->is(tok::comma) &&
+            CurrentToken->Previous->isNot(tok::kw_operator))
+          break;
         if (CurrentToken && CurrentToken->Previous->isOneOf(
                                 TT_BinaryOperator, TT_UnaryOperator, tok::comma,
                                 tok::star, tok::arrow, tok::amp, tok::ampamp))
           CurrentToken->Previous->setType(TT_OverloadedOperator);
       }
-      if (CurrentToken) {
+      if (CurrentToken && CurrentToken->is(tok::l_paren))
         CurrentToken->setType(TT_OverloadedOperatorLParen);
-        if (CurrentToken->Previous->is(TT_BinaryOperator))
-          CurrentToken->Previous->setType(TT_OverloadedOperator);
-      }
+      if (CurrentToken && CurrentToken->Previous->is(TT_BinaryOperator))
       break;
     case tok::question:
       if (Tok->is(TT_CSharpNullConditionalLSquare)) {
         if (!parseSquare())
           return false;
         break;
       }
       if (Tok->isOneOf(TT_CSharpNullConditional, TT_CSharpNullCoalescing))

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, OperatorSpacing) {
   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);
 }
 
+TEST_F(FormatTest, OperatorPassedAsAFunctionPtr) {
+  FormatStyle Style = getLLVMStyle();
+  // PR46157
+  verifyFormat("foo(operator+, -42);", Style);
+  verifyFormat("foo(operator++, -42);", Style);
+  verifyFormat("foo(operator--, -42);", Style);
+  verifyFormat("foo(-42, operator--);", Style);
+  verifyFormat("foo(-42, operator, );", Style);
+  verifyFormat("foo(operator, , -42);", Style);
+}
+
 TEST_F(FormatTest, VeryLongNamespaceCommentSplit) {
   // These tests are not in NamespaceFixer because that doesn't
   // test its interaction with line wrapping
   FormatStyle Style = getLLVMStyle();
   Style.ColumnLimit = 80;
   verifyFormat("namespace {\n"
                "int i;\n"
                "int j;\n"