[clang-format/ObjC] Fix counting selector name parts for ObjC

[jolesiak]

Summary

Counts selector parts also for method declarations and counts correctly for methods without arguments.
This is an internal change and doesn't influence formatting on its own (at the current state). Its lack would be visible after applying D48719.

[jolesiak]

Fix comment

[benhamilton]

Count selector parts also for method declarations.

What bug does this fix? Can you add a test which breaks before this change and is fixed by this change?

lib/Format/FormatToken.h

247–248 ↗

(On Diff #153298)

Grammar nit: that whole selector consist of -> that the whole selector consists of

lib/Format/TokenAnnotator.cpp

519 ↗

(On Diff #153298)

Grammar nit-pick: when method -> when the method

520 ↗

(On Diff #153298)

expression -> method call

627 ↗

(On Diff #153298)

Grammar nit-pick: methods number -> methods, the number

628 ↗

(On Diff #153298)

Grammar nit-picks:

• have different structure -> have a different structure
• parameters are not -> the parameters are not

628–629 ↗

(On Diff #153298)

Why does parenthesis scope matter here? updateParameterCount() is called from parseSquare().

I'm not sure what the goal of this change is.

[jolesiak]

lib/Format/FormatToken.h

247–248 ↗

(On Diff #153298)

Thanks!

lib/Format/TokenAnnotator.cpp

519 ↗

(On Diff #153298)

Thanks!

520 ↗

(On Diff #153298)

Thanks!

627 ↗

(On Diff #153298)

Thanks!

628 ↗

(On Diff #153298)

Thanks!

628–629 ↗

(On Diff #153298)

My bad, I thought the word "parenthesis" can also mean any of "(", "[", "{", "<" (not only specifically the first one).
I've replaced with "a bracket scope".

I'll comment on the goal of this change in non-inline comment.

[jolesiak]

In D48716#1146796, @benhamilton wrote:

Count selector parts also for method declarations.

What bug does this fix? Can you add a test which breaks before this change and is fixed by this change?

Sorry for the confusion.

General comment to changes D48716, D48718, D48719, D48720 (which are the split of D48352):
These changes are separate, in the sense that they fix different issues. However, they should be chained as every change (apart from the first one) is based on previous ones: D48716 -> D48718 -> D48719 -> D48720. I don't know how to chain them in Phabricator (I should probably leave some comments about what every change is based on though).

D48716 fixes the mechanism of counting parameters. This is an internal change though and doesn't influence formatting on its own (at the current state). Its lack would be visible after applying D48719. Therefore, I'm not aware of any formatting test that fails before applying this change and succeeds after. As far as I know internal functions/methods are not tested at all. Thus, the tests are part of D48719.

This is why initially I put everything into one change: D48352, but I agree that it was not readable at all.

[jolesiak]

Fix grammar mistakes.

[benhamilton]

In D48716#1149293, @jolesiak wrote:

General comment to changes D48716, D48718, D48719, D48720 (which are the split of D48352):

These changes are separate, in the sense that they fix different issues. However, they should be chained as every change (apart from the first one) is based on previous ones: D48716 -> D48718 -> D48719 -> D48720. I don't know how to chain them in Phabricator (I should probably leave some comments about what every change is based on though).

Phabricator has an Edit Related Revisions feature where you can tag other revisions as being dependencies of or depending upon the current revision:

I went ahead and used it to link the revisions in the order you listed above. (I also like to edit the 1-line description and add [1/x] at the beginning — but that's up to you.)

D48716 fixes the mechanism of counting parameters. This is an internal change though and doesn't influence formatting on its own (at the current state). Its lack would be visible after applying D48719. Therefore, I'm not aware of any formatting test that fails before applying this change and succeeds after. As far as I know internal functions/methods are not tested at all. Thus, the tests are part of D48719.

Great. Just mentioning that in the diff description should be fine.

[jolesiak]

In D48716#1149733, @benhamilton wrote:

Phabricator has an Edit Related Revisions feature where you can tag other revisions as being dependencies of or depending upon the current revision:

Thanks! That's what I was looking for.

cfe/trunk/lib/Format/FormatToken.h

struct FormatToken {
 
   /// If this is the first ObjC selector name in an ObjC method
   /// definition or call, this contains the length of the longest name.
   ///
   /// This being set to 0 means that the selectors should not be colon-aligned,
   /// e.g. because several of them are block-type.
   unsigned LongestObjCSelectorName = 0;
 
-  /// How many parts ObjC selector have (i.e. how many parameters method
-  /// has).
+  /// If this is the first ObjC selector name in an ObjC method
+  /// definition or call, this contains the number of parts that the whole
+  /// selector consist of.
   unsigned ObjCSelectorNameParts = 0;
 
   /// Stores the number of required fake parentheses and the
   /// corresponding operator precedence.
   ///
   /// If multiple fake parentheses start at a token, this vector stores them in
   /// reverse order, i.e. inner fake parenthesis first.
   SmallVector<prec::Level, 4> FakeLParens;

cfe/trunk/lib/Format/TokenAnnotator.cpp

while (CurrentToken) {
           // determineStarAmpUsage() thinks that '*' '[' is allocating an
           // array of pointers, but if '[' starts a selector then '*' is a
           // binary operator.
           if (Parent && Parent->is(TT_PointerOrReference))
             Parent->Type = TT_BinaryOperator;
         }
         Left->MatchingParen = CurrentToken;
         CurrentToken->MatchingParen = Left;
+        // FirstObjCSelectorName is set when a colon is found. This does
+        // not work, however, when the method has no parameters.
+        // Here, we set FirstObjCSelectorName when the end of the method call is
+        // reached, in case it was not set already.
+        if (!Contexts.back().FirstObjCSelectorName) {
+            FormatToken* Previous = CurrentToken->getPreviousNonComment();
+            if (Previous && Previous->is(TT_SelectorName)) {
+              Previous->ObjCSelectorNameParts = 1;
+              Contexts.back().FirstObjCSelectorName = Previous;
+            }
+        } else {
+          Left->ParameterCount =
+              Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
+        }
         if (Contexts.back().FirstObjCSelectorName) {
           Contexts.back().FirstObjCSelectorName->LongestObjCSelectorName =
               Contexts.back().LongestObjCSelectorName;
-          Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts =
-              Left->ParameterCount;
           if (Left->BlockParameterCount > 1)
             Contexts.back().FirstObjCSelectorName->LongestObjCSelectorName = 0;
         }
         next();
         return true;
       }
       if (CurrentToken->isOneOf(tok::r_paren, tok::r_brace))
         return false;
       if (CurrentToken->is(tok::colon)) {
         if (IsCpp11AttributeSpecifier &&
             CurrentToken->endsSequence(tok::colon, tok::identifier,
                                        tok::kw_using)) {
           // Remember that this is a [[using ns: foo]] C++ attribute, so we
           // don't add a space before the colon (unlike other colons).
           CurrentToken->Type = TT_AttributeColon;
         } else if (Left->isOneOf(TT_ArraySubscriptLSquare,
                                  TT_DesignatedInitializerLSquare)) {
           Left->Type = TT_ObjCMethodExpr;
           StartsObjCMethodExpr = true;
-          // ParameterCount might have been set to 1 before expression was
-          // recognized as ObjCMethodExpr (as '1 + number of commas' formula is
-          // used for other expression types). Parameter counter has to be,
-          // therefore, reset to 0.
-          Left->ParameterCount = 0;
           Contexts.back().ColonIsObjCMethodExpr = true;
           if (Parent && Parent->is(tok::r_paren))
             // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
             Parent->Type = TT_CastRParen;
         }
         ColonFound = true;
       }
       if (CurrentToken->is(tok::comma) && Left->is(TT_ObjCMethodExpr) &&
if (CurrentToken) {
         if (!consumeToken())
           return false;
       }
     }
     return true;
   }
 
   void updateParameterCount(FormatToken *Left, FormatToken *Current) {
+    // For ObjC methods, the number of parameters is calculated differently as
+    // method declarations have a different structure (the parameters are not
+    // inside a bracket scope).
     if (Current->is(tok::l_brace) && Current->BlockKind == BK_Block)
       ++Left->BlockParameterCount;
-    if (Left->Type == TT_ObjCMethodExpr) {
-      if (Current->is(tok::colon))
-        ++Left->ParameterCount;
-    } else if (Current->is(tok::comma)) {
+    if (Current->is(tok::comma)) {
       ++Left->ParameterCount;
       if (!Left->Role)
         Left->Role.reset(new CommaSeparatedList(Style));
       Left->Role->CommaFound(Current);
     } else if (Left->ParameterCount == 0 && Current->isNot(tok::comment)) {
       Left->ParameterCount = 1;
     }
   }
case tok::colon:
             UnknownIdentifierInMethodDeclaration) {
           Tok->Previous->Type = TT_SelectorName;
           if (!Contexts.back().FirstObjCSelectorName)
             Contexts.back().FirstObjCSelectorName = Tok->Previous;
           else if (Tok->Previous->ColumnWidth >
                    Contexts.back().LongestObjCSelectorName)
             Contexts.back().LongestObjCSelectorName =
                 Tok->Previous->ColumnWidth;
+          ++Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
         }
       } else if (Contexts.back().ColonIsForRangeExpr) {
         Tok->Type = TT_RangeBasedForLoopColon;
       } else if (CurrentToken && CurrentToken->is(tok::numeric_constant)) {
         Tok->Type = TT_BitFieldColon;
       } else if (Contexts.size() == 1 &&
                  !Line.First->isOneOf(tok::kw_enum, tok::kw_case)) {
         if (Tok->getPreviousNonComment()->isOneOf(tok::r_paren,