[clang-format] regressed default behavior for operator parentheses

[MyDeveloperDay]

Summary

D110833: [clang-format] Refactor SpaceBeforeParens to add options regressed behavior of spaces before parentheses for operators, this revision reverts that so that operators are handled as they were before.

I think in hindsight it was a mistake to try and consume operator behaviour in with the function behaviour, I think Operators can be considered a special style. Its seems the code is getting confused as to if this is a function declaration or definition.

I think latterly we can consider adding an operator parentheses specific custom option but this should have been explicitly called out as it can impact projects.

[curdeius]

LGTM

[MyDeveloperDay]

@crayroud I'm going to commit this change to get past the regression, but we can continue to discuss here how you'd like to proceed.

[crayroud]

clang/unittests/Format/FormatTest.cpp

14126–14127

Why did you comment these lines? When SpaceBeforeParens is set to always it is not correct to have a space before the opening parenthesis, for operators ?

clang/lib/Format/TokenAnnotator.cpp

if (Left.is(tok::semi))
       return true;
     if (Left.isOneOf(tok::pp_elif, tok::kw_for, tok::kw_while, tok::kw_switch,
                      tok::kw_case, TT_ForEachMacro, TT_ObjCForIn))
       return Style.SpaceBeforeParensOptions.AfterControlStatements ||
              spaceRequiredBeforeParens(Right);
     if (Left.isIf(Line.Type != LT_PreprocessorDirective))
       return Style.SpaceBeforeParensOptions.AfterControlStatements ||
              spaceRequiredBeforeParens(Right);
+
+    // TODO add Operator overloading specific Options to
+    // SpaceBeforeParensOptions
+    if (Right.is(TT_OverloadedOperatorLParen))
+      return spaceRequiredBeforeParens(Right);
     // Function declaration or definition
-    if (Line.MightBeFunctionDecl && (Left.is(TT_FunctionDeclarationName) ||
-                                     Right.is(TT_OverloadedOperatorLParen))) {
+    if (Line.MightBeFunctionDecl && (Left.is(TT_FunctionDeclarationName))) {
       if (Line.mightBeFunctionDefinition())
         return Style.SpaceBeforeParensOptions.AfterFunctionDefinitionName ||
                spaceRequiredBeforeParens(Right);
       else
         return Style.SpaceBeforeParensOptions.AfterFunctionDeclarationName ||
                spaceRequiredBeforeParens(Right);
     }
     // Lambda

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsOverloadedOperators) {
   verifyFormat("void f() { a.operator*(); }");
   verifyFormat("void f() { a.operator*(b & b); }");
   verifyFormat("void f() { a->operator&(a * b); }");
   verifyFormat("void f() { NS::a.operator+(*b * *b); }");
   // TODO: Calling an operator as a non-member function is hard to distinguish.
   // https://llvm.org/PR50629
   // verifyFormat("void f() { operator*(a & a); }");
   // verifyFormat("void f() { operator&(a, b * b); }");
+
+  verifyFormat("::operator delete(foo);");
+  verifyFormat("::operator new(n * sizeof(foo));");
+  verifyFormat("foo() { ::operator delete(foo); }");
+  verifyFormat("foo() { ::operator new(n * sizeof(foo)); }");
 }
 
 TEST_F(FormatTest, UnderstandsFunctionRefQualification) {
   verifyFormat("Deleted &operator=(const Deleted &) & = default;");
   verifyFormat("Deleted &operator=(const Deleted &) && = delete;");
   verifyFormat("SomeType MemberFunction(const Deleted &) & = delete;");
   verifyFormat("SomeType MemberFunction(const Deleted &) && = delete;");
   verifyFormat("Deleted &operator=(const Deleted &) &;");
TEST_F(FormatTest, ConfigurableSpaceBeforeParens) {
   verifyFormat("auto f (int x) -> _Atomic (x);", Space);
   verifyFormat("auto f (int x) -> __underlying_type (x);", Space);
   verifyFormat("int f (T x) noexcept (x.create ());", Space);
   verifyFormat("alignas (128) char a[128];", Space);
   verifyFormat("size_t x = alignof (MyType);", Space);
   verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");", Space);
   verifyFormat("int f () throw (Deprecated);", Space);
   verifyFormat("typedef void (*cb) (int);", Space);
-  verifyFormat("T A::operator() ();", Space);
-  verifyFormat("X A::operator++ (T);", Space);
+  // FIXME these tests regressed behaviour.
+  // verifyFormat("T A::operator() ();", Space);
+  // verifyFormat("X A::operator++ (T);", Space);
   verifyFormat("auto lambda = [] () { return 0; };", Space);
   verifyFormat("int x = int (y);", Space);
 
   FormatStyle SomeSpace = getLLVMStyle();
   SomeSpace.SpaceBeforeParens = FormatStyle::SBPO_NonEmptyParentheses;
 
   verifyFormat("[]() -> float {}", SomeSpace);
   verifyFormat("[] (auto foo) {}", SomeSpace);
TEST_F(FormatTest, ConfigurableSpaceBeforeParens) {
   verifyFormat("int f (T x) noexcept (x.create());", SomeSpace);
   verifyFormat("alignas (128) char a[128];", SomeSpace);
   verifyFormat("size_t x = alignof (MyType);", SomeSpace);
   verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
                SomeSpace);
   verifyFormat("int f() throw (Deprecated);", SomeSpace);
   verifyFormat("typedef void (*cb) (int);", SomeSpace);
   verifyFormat("T A::operator()();", SomeSpace);
-  verifyFormat("X A::operator++ (T);", SomeSpace);
+  // FIXME these tests regressed behaviour.
+  // verifyFormat("X A::operator++ (T);", SomeSpace);
   verifyFormat("int x = int (y);", SomeSpace);
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 
   FormatStyle SpaceControlStatements = getLLVMStyle();
   SpaceControlStatements.SpaceBeforeParens = FormatStyle::SBPO_Custom;
   SpaceControlStatements.SpaceBeforeParensOptions.AfterControlStatements = true;
 
   verifyFormat("while (true)\n"
TEST_F(FormatTest, ConfigurableSpaceBeforeParens) {
   verifyFormat("auto f (int x) -> __underlying_type(x);", SpaceFuncDecl);
   verifyFormat("int f (T x) noexcept(x.create());", SpaceFuncDecl);
   verifyFormat("alignas(128) char a[128];", SpaceFuncDecl);
   verifyFormat("size_t x = alignof(MyType);", SpaceFuncDecl);
   verifyFormat("static_assert(sizeof(char) == 1, \"Impossible!\");",
                SpaceFuncDecl);
   verifyFormat("int f () throw(Deprecated);", SpaceFuncDecl);
   verifyFormat("typedef void (*cb)(int);", SpaceFuncDecl);
-  verifyFormat("T A::operator() ();", SpaceFuncDecl);
-  verifyFormat("X A::operator++ (T);", SpaceFuncDecl);
+  // FIXME these tests regressed behaviour.
+  // verifyFormat("T A::operator() ();", SpaceFuncDecl);
+  // verifyFormat("X A::operator++ (T);", SpaceFuncDecl);
   verifyFormat("T A::operator()() {}", SpaceFuncDecl);
   verifyFormat("auto lambda = []() { return 0; };", SpaceFuncDecl);
   verifyFormat("int x = int(y);", SpaceFuncDecl);
   verifyFormat("M(std::size_t R, std::size_t C) : C(C), data(R) {}",
                SpaceFuncDecl);
 
   FormatStyle SpaceFuncDef = getLLVMStyle();
   SpaceFuncDef.SpaceBeforeParens = FormatStyle::SBPO_Custom;
TEST_F(FormatTest, ConfigurableSpaceBeforeParens) {
   verifyFormat("alignas(128) char a[128];", SpaceFuncDef);
   verifyFormat("size_t x = alignof(MyType);", SpaceFuncDef);
   verifyFormat("static_assert(sizeof(char) == 1, \"Impossible!\");",
                SpaceFuncDef);
   verifyFormat("int f() throw(Deprecated);", SpaceFuncDef);
   verifyFormat("typedef void (*cb)(int);", SpaceFuncDef);
   verifyFormat("T A::operator()();", SpaceFuncDef);
   verifyFormat("X A::operator++(T);", SpaceFuncDef);
-  verifyFormat("T A::operator() () {}", SpaceFuncDef);
+  // verifyFormat("T A::operator() () {}", SpaceFuncDef);
   verifyFormat("auto lambda = [] () { return 0; };", SpaceFuncDef);
   verifyFormat("int x = int(y);", SpaceFuncDef);
   verifyFormat("M(std::size_t R, std::size_t C) : C(C), data(R) {}",
                SpaceFuncDef);
 
   FormatStyle SpaceIfMacros = getLLVMStyle();
   SpaceIfMacros.IfMacros.clear();
   SpaceIfMacros.IfMacros.push_back("MYIF");
TEST_F(FormatTest, ConfigurableSpaceBeforeParens) {
   verifyFormat("int f (T x) noexcept (x.create());", SomeSpace2);
   verifyFormat("alignas (128) char a[128];", SomeSpace2);
   verifyFormat("size_t x = alignof (MyType);", SomeSpace2);
   verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
                SomeSpace2);
   verifyFormat("int f() throw (Deprecated);", SomeSpace2);
   verifyFormat("typedef void (*cb) (int);", SomeSpace2);
   verifyFormat("T A::operator()();", SomeSpace2);
-  verifyFormat("X A::operator++ (T);", SomeSpace2);
+  // verifyFormat("X A::operator++ (T);", SomeSpace2);
   verifyFormat("int x = int (y);", SomeSpace2);
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace2);
 }
 
 TEST_F(FormatTest, SpaceAfterLogicalNot) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpaceAfterLogicalNot = true;