[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 ↗

(On Diff #140122)

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 ↗

(On Diff #140122)

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 ↗

(On Diff #140122)

OK, done.

904 ↗

(On Diff #140122)

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 ↗

(On Diff #140122)

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 ↗

(On Diff #140122)

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

Yes, we can remove these lines now. Done.

[djasper]

Looks good.

cfe/trunk/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 = State.Stack.back().Indent;
+      if (Style.IndentWrappedFunctionNames)
+        MinIndent = std::max(MinIndent,
+                             State.FirstIndent + Style.ContinuationIndentWidth);
+      // If LongestObjCSelectorName is 0, we are indenting the first
+      // part of an ObjC selector (or a selector component which is
+      // not colon-aligned due to block formatting).
+      //
+      // Otherwise, we are indenting a subsequent part of an ObjC
+      // selector which should be colon-aligned to the longest
+      // component of the ObjC selector.
+      //
+      // In either case, we want to respect Style.IndentWrappedFunctionNames.
+      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;

cfe/trunk/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"