[clang-format] Ensure wrapped ObjC selectors with 1 arg obey IndentWrappedFunctionNames

[benhamilton]

Summary

In D43121, @Typz introduced logic to avoid indenting 2-or-more
argument ObjC selectors too far to the right if the first component
of the selector was longer than the others.

This had a small side effect of causing wrapped ObjC selectors with
exactly 1 argument to not obey IndentWrappedFunctionNames:

- (aaaaaaaaaa)
aaaaaaaaaa;

This diff fixes the issue by ensuring we align wrapped 1-argument
ObjC selectors correctly:

- (aaaaaaaaaa)
    aaaaaaaaaa;

Test Plan: New tests added. Test failed before change, passed

after change. Ran tests with:
% make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests

[jolesiak]

Well spotted.

[djasper]

lib/Format/ContinuationIndenter.cpp

899

I think I'd now find this slightly easier to read as:

unsigned MinIndent = State.Stack.back().Indent;
if (Style.IndentWrappedFunctionNames)
  MinIndent = std::max(MinIndent, State.FirstIndent + Style.ContinuationIndentWidth);

904

Does this if actually change the behavior in any way? With LongestObjCSelectorName being 0, isn't that:

return MinIndent +
       std::max(0, ColumnWidth) - ColumnWidth;

(and with ColumnWidth being >= 0, this should be just MinIndent)

[benhamilton]

@djasper comments

[benhamilton]

Thanks, applied your suggested change.

lib/Format/ContinuationIndenter.cpp

899

OK, done.

904

The - ColumnWidth part is only for the case where LongestObjCSelectorName is *not* 0. If it's 0, we return MinIndent which ensures we obey Style.IndentWrappedFunctionNames.

The problem with the code before this diff is when LongestObjCSelectorName was 0, we ignored Style.IndentWrappedFunctionNames and always returned State.Stack.back().Indent regardless of that setting.

After this diff, when LongestObjCSelectorName is 0 (i.e., this is either the first part of the selector or a selector which is not colon-aligned due to block formatting), we change the behavior to indent to at least State.FirstIndent + Style.ContinuationIndentWidth, like all other indentation logic in this file.

I've added some comments explaining what's going on, since this code is quite complex.

[benhamilton]

One more tidy-up

[djasper]

lib/Format/ContinuationIndenter.cpp

904

I am not saying your change is wrong. And I might be getting out of practice with coding. My question is, what changes if you remove lines 906 and 907 (the "if (...) return MinIndent")?

[benhamilton]

Remove if (...) return MinIndent.

[benhamilton]

lib/Format/ContinuationIndenter.cpp

904

Oh, I see what you're saying now! Thanks for clarifying.

Yes, we can remove these lines now. Done.

[djasper]

Looks good.

lib/Format/ContinuationIndenter.cpp

if ((PreviousNonComment &&
         PreviousNonComment->isOneOf(
             TT_AttributeParen, TT_AttributeSquare, TT_FunctionAnnotationRParen,
             TT_JavaAnnotation, TT_LeadingJavaAnnotation))) ||
       (!Style.IndentWrappedFunctionNames &&
        NextNonComment->isOneOf(tok::kw_operator, TT_FunctionDeclarationName)))
     return std::max(State.Stack.back().LastSpace, State.Stack.back().Indent);
   if (NextNonComment->is(TT_SelectorName)) {
     if (!State.Stack.back().ObjCSelectorNameFound) {
-      if (NextNonComment->LongestObjCSelectorName == 0)
-        return State.Stack.back().Indent;
-      return (Style.IndentWrappedFunctionNames
-                  ? std::max(State.Stack.back().Indent,
-                             State.FirstIndent + Style.ContinuationIndentWidth)
-                  : State.Stack.back().Indent) +
+      unsigned MinIndent =
+          (Style.IndentWrappedFunctionNames
+               : State.Stack.back().Indent);
+      if (NextNonComment->LongestObjCSelectorName == 0)
+        return MinIndent;
+      return MinIndent +
              std::max(NextNonComment->LongestObjCSelectorName,
                       NextNonComment->ColumnWidth) -
              NextNonComment->ColumnWidth;
     }
     if (!State.Stack.back().AlignColons)
       return State.Stack.back().Indent;
     if (State.Stack.back().ColonPos > NextNonComment->ColumnWidth)
       return State.Stack.back().ColonPos - NextNonComment->ColumnWidth;

unittests/Format/FormatTestObjC.cpp

verifyFormat("- (void)shortf:(GTMFoo *)theFoo\n"
                "    dontAlignNamef:(NSRect)theRect {\n"
                "}");
 
   // Make sure we don't break aligning for short parameter names.
   verifyFormat("- (void)shortf:(GTMFoo *)theFoo\n"
                "       aShortf:(NSRect)theRect {\n"
                "}");
 
+  // Make sure selectors with 0, 1, or more arguments are indented
+  // when IndentWrappedFunctionNames is true.
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaa;\n");
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "     aaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a\n"
+               "     aaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
+
   // Format pairs correctly.
   Style.ColumnLimit = 80;
   verifyFormat("- (void)drawRectOn:(id)surface\n"
                "            ofSize:(aaaaaaaa)height\n"
                "                  :(size_t)width\n"
                "          atOrigin:(size_t)x\n"
                "                  :(size_t)y\n"
                "             aaaaa:(a)yyy\n"