Allow PenaltyBreakBeforeFirstCallParameter to not break Obj-C call before first parameter

[kmensah]

Summary

Patch to prefer not breaking before the first parameter on an Objective-c method call if possible.

[djasper]

lib/Format/TokenAnnotator.cpp

1751

I think this doesn't work yet, as the callee can also be the result of an expression, right?

so: [[self a] a:b c:d];

But I think you can just check whether Right.LongestObjCSelectorName is != 0. This value should only be set for the first selector.

[kmensah]

Fixing per djasper's comments.

[kmensah]

Fixing obj-c method calls with no parameters and and adding unit test for when callee is an expression.

[kmensah]

Ready for review again.

[djasper]

lib/Format/TokenAnnotator.cpp

1701

I don't understand this change. Could you elaborate?

unittests/Format/FormatTest.cpp

7343–7344

No, this is exactly as many spaces as intended. It is 4 spaces from "function".

[kmensah]

Fixing per comments from djasper

[kmensah]

Responding to comments form djasper

lib/Format/TokenAnnotator.cpp

1701

without this single Objective-C calls with no parameters ([self foo]) didn't have PenaltyBreakBeforeFirstCallParameter applied to stop a break before "foo". I wanted to return the max between the old default of 200 and PenaltyBreakBeforeFirstCallParameter but didn't want to have specify 200 multiple times.

unittests/Format/FormatTest.cpp

7343–7344

Removed that statement.

lib/Format/TokenAnnotator.cpp

       return 110;
     if (InFunctionDecl && Right.NestingLevel == 0)
       return Style.PenaltyReturnTypeOnItsOwnLine;
-    return 200;
+    unsigned penalty = 200;
+    if(Right.is(TT_StartOfName)) {
+      // Apply the break penalty for objective-c methods with with no parameters.
+      penalty = std::max(penalty, Style.PenaltyBreakBeforeFirstCallParameter);
+    }
+    return penalty;
   }
   if (Right.is(TT_PointerOrReference))
     return 190;

 
   // In Objective-C method expressions, prefer breaking before "param:" over
   // breaking after it.
-  if (Right.is(TT_SelectorName))
+  if (Right.is(TT_SelectorName)) {
+    // While we prefer breaking before "param:", if this is the first paramater
+    // apply PenaltyBreakBeforeFirstCallParameter
+    if (Right.LongestObjCSelectorName != 0) {
+      return Style.PenaltyBreakBeforeFirstCallParameter;
+    }
     return 0;
+  }
+
   if (Left.is(tok::colon) && Left.is(TT_ObjCMethodExpr))
     return Line.MightBeFunctionDecl ? 50 : 500;
 

unittests/Format/FormatTest.cpp

                "    aaa:aaa];");
   verifyFormat("bool a = ([aaaaaaaa aaaaa] == aaaaaaaaaaaaaaaaa ||\n"
                "          [aaaaaaaa aaaaa] == aaaaaaaaaaaaaaaaaaaa);");
+
+  // Allow PenaltyBreakBeforeFirstCallParameter to stop us from breaking before
+  // the first selector name.
+  FormatStyle selectorBreakStyle = getGoogleStyleWithColumns(60);
+  selectorBreakStyle.PenaltyBreakBeforeFirstCallParameter = 10000;
+  verifyFormat("[self function1:[OtherType function2:longParamaterName1\n"
+               "                               param:longParameterName2]];",
+               selectorBreakStyle);
+
+  // But don't break if the callee is an expression
+  /*TODO: This is what it should ideally look like.
+    verifyFormat(
+      "[[self foo] function:[OtherType function2:longParamaterName\n"
+      "                                    param:longParameterName2]];",
+      selectorBreakStyle);
+  */
+
+  // TODO: logic in ContinuationIndenter::moveStatePastFakeLParens adds more
+  // spaces than expected to second line.
+  verifyFormat("[[self foo] function:\n"
+               "                [OtherType function2:longParamaterName\n"
+               "                               param:longParameterName2]];",
+               selectorBreakStyle);
 }
 
 TEST_F(FormatTest, ObjCAt) {