clang-format: Fix return type breaking with operator overload functions

[djasper]

lib/Format/TokenAnnotator.cpp

509 ↗

(On Diff #29647)

Why is this last check important? To not mark operator()()()? Is that important?

1478 ↗

(On Diff #29647)

What kinds of tokens do you expect to consume here? Would it be better to be more strict, i.e. consume exactly an operator, "new", "delete", "()" and such?

1492 ↗

(On Diff #29647)

This is very similar to the above. What are the exact differences (maybe add a comment)? Could we factor some of this out into a local function or a lambda?

1495 ↗

(On Diff #29647)

According to LLVM coding standards, no "else" after "return", "break" or "continue".

2187 ↗

(On Diff #29647)

I think this is incorrect as we currently break after the coloncolon if a nested name specifier has to be split, e.g.:

void AssumeThisIsAVeryLongTypeOrSetASmallColumnLimit::
    operator+() {
}

[poiru]

lib/Format/TokenAnnotator.cpp

509 ↗

(On Diff #29647)

Probably not, removed.

1478 ↗

(On Diff #29647)

Done.

1492 ↗

(On Diff #29647)

Done.

2187 ↗

(On Diff #29647)

Removed this bit, thanks.

[poiru]

Addressed review comments.

[djasper]

Looks good. Thank you!

cfe/trunk/lib/Format/ContinuationIndenter.cpp

if (Current.NestingLevel == 0 && !Current.isTrailingComment()) {
     if (Previous.is(TT_FunctionAnnotationRParen))
       return true;
     if (Previous.is(TT_LeadingJavaAnnotation) && Current.isNot(tok::l_paren) &&
         Current.isNot(TT_LeadingJavaAnnotation))
       return true;
   }
 
   // If the return type spans multiple lines, wrap before the function name.
-  if (Current.isOneOf(TT_FunctionDeclarationName, tok::kw_operator) &&
+  if ((Current.is(TT_FunctionDeclarationName) ||
+       (Current.is(tok::kw_operator) && !Previous.is(tok::coloncolon))) &&
       State.Stack.back().BreakBeforeParameter)
     return true;
 
   if (startsSegmentOfBuilderTypeCall(Current) &&
       (State.Stack.back().CallContinuation != 0 ||
        State.Stack.back().BreakBeforeParameter))
     return true;
 

cfe/trunk/lib/Format/TokenAnnotator.cpp

case tok::kw_while:
       break;
     case tok::kw_for:
       Contexts.back().ColonIsForRangeExpr = true;
       next();
       if (!parseParens())
         return false;
       break;
     case tok::l_paren:
+      // When faced with 'operator()()', the kw_operator handler incorrectly
+      // marks the first l_paren as a OverloadedOperatorLParen. Here, we make
+      // the first two parens OverloadedOperators and the second l_paren an
+      // OverloadedOperatorLParen.
+      if (Tok->Previous &&
+          Tok->Previous->is(tok::r_paren) &&
+          Tok->Previous->MatchingParen &&
+          Tok->Previous->MatchingParen->is(TT_OverloadedOperatorLParen)) {
+        Tok->Previous->Type = TT_OverloadedOperator;
+        Tok->Previous->MatchingParen->Type = TT_OverloadedOperator;
+        Tok->Type = TT_OverloadedOperatorLParen;
+      }
+
       if (!parseParens())
         return false;
       if (Line.MustBeDeclaration && Contexts.size() == 1 &&
           !Contexts.back().IsExpression && !Line.startsWith(TT_ObjCProperty) &&
           (!Tok->Previous ||
            !Tok->Previous->isOneOf(tok::kw_decltype, tok::kw___attribute,
                                    TT_LeadingJavaAnnotation)))
         Line.MightBeFunctionDecl = true;
void TokenAnnotator::annotate(AnnotatedLine &Line) {
 
   Line.First->SpacesRequiredBefore = 1;
   Line.First->CanBreakBefore = Line.First->MustBreakBefore;
 }
 
 // This function heuristically determines whether 'Current' starts the name of a
 // function declaration.
 static bool isFunctionDeclarationName(const FormatToken &Current) {
+  auto skipOperatorName = [](const FormatToken* Next) -> const FormatToken* {
+    for (; Next; Next = Next->Next) {
+      if (Next->is(TT_OverloadedOperatorLParen))
+        return Next;
+      if (Next->is(TT_OverloadedOperator))
+        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))
+          Next = Next->Next->Next;
+        continue;
+      }
+
+      break;
+    }
+    return nullptr;
+  };
+
+  const FormatToken *Next = Current.Next;
+  if (Current.is(tok::kw_operator)) {
+    if (Current.Previous && Current.Previous->is(tok::coloncolon))
+      return false;
+    Next = skipOperatorName(Next);
+  } else {
     if (!Current.is(TT_StartOfName) || Current.NestingLevel != 0)
       return false;
-  const FormatToken *Next = Current.Next;
     for (; Next; Next = Next->Next) {
       if (Next->is(TT_TemplateOpener)) {
         Next = Next->MatchingParen;
       } else if (Next->is(tok::coloncolon)) {
         Next = Next->Next;
-      if (!Next || !Next->is(tok::identifier))
+        if (!Next)
+          return false;
+        if (Next->is(tok::kw_operator)) {
+          Next = skipOperatorName(Next->Next);
+          break;
+        }
+        if (!Next->is(tok::identifier))
           return false;
       } else if (Next->is(tok::l_paren)) {
         break;
       } else {
         return false;
       }
     }
-  if (!Next)
+  }
+
+  if (!Next || !Next->is(tok::l_paren))
     return false;
-  assert(Next->is(tok::l_paren));
   if (Next->Next == Next->MatchingParen)
     return true;
   for (const FormatToken *Tok = Next->Next; Tok && Tok != Next->MatchingParen;
        Tok = Tok->Next) {
     if (Tok->is(tok::kw_const) || Tok->isSimpleTypeSpecifier() ||
         Tok->isOneOf(TT_PointerOrReference, TT_StartOfName))
       return true;
     if (Tok->isOneOf(tok::l_brace, tok::string_literal, TT_ObjCMethodExpr) ||

cfe/trunk/unittests/Format/FormatTest.cpp

verifyFormat("const char *\n"
                Style);
   verifyFormat("template <class T>\n"
                "T *\n"
                "f(T &c) {\n" // Break here.
                "  return NULL;\n"
                "}\n"
                "template <class T> T *f(T &c);\n", // No break here.
                Style);
+  verifyFormat("class C {\n"
+               "  int\n"
+               "  operator+() {\n"
+               "    return 1;\n"
+               "  }\n"
+               "  int\n"
+               "  operator()() {\n"
+               "    return 1;\n"
+               "  }\n"
+               "};\n",
+               Style);
+  verifyFormat("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);
+  verifyFormat("void *\n"
+               "operator new(std::size_t s) {}",
+               Style);
+  verifyFormat("void *\n"
+               "operator delete[](void *ptr) {}",
+               Style);
   Style.BreakBeforeBraces = FormatStyle::BS_Stroustrup;
   verifyFormat("const char *\n"
                "f(void)\n" // Break here.
                "{\n"
                "  return \"\";\n"
                "}\n"
                "const char *bar(void);\n", // No break here.
                Style);