[PATCH] D48716: [clang-format] Fix counting parameters/arguments for ObjC

2018-07-02 Thread Ben Hamilton via Phabricator via cfe-commits 
benhamilton accepted this revision.
benhamilton added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D48716#1149293, @jolesiak wrote:

> General comment to changes https://reviews.llvm.org/D48716, 
> https://reviews.llvm.org/D48718, https://reviews.llvm.org/D48719, 
> https://reviews.llvm.org/D48720 (which are the split of 
> https://reviews.llvm.org/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: https://reviews.llvm.org/D48716 -> 
> https://reviews.llvm.org/D48718 -> https://reviews.llvm.org/D48719 -> 
> https://reviews.llvm.org/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:

F6560886: Screen Shot 2018-07-02 at 10.57.37 AM.png 


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.)

> https://reviews.llvm.org/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 
> https://reviews.llvm.org/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 https://reviews.llvm.org/D48719.

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


Repository:
  rC Clang

https://reviews.llvm.org/D48716



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48716: [clang-format] Fix counting parameters/arguments for ObjC

2018-07-02 Thread Jacek Olesiak via Phabricator via cfe-commits 
jolesiak updated this revision to Diff 153689.
jolesiak added a comment.

Fix grammar mistakes.


Repository:
  rC Clang

https://reviews.llvm.org/D48716

Files:
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -515,11 +515,23 @@
 }
 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;
 }
@@ -539,11 +551,6 @@
  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.
@@ -617,12 +624,12 @@
   }
 
   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));
@@ -712,6 +719,7 @@
Contexts.back().LongestObjCSelectorName)
 Contexts.back().LongestObjCSelectorName =
 Tok->Previous->ColumnWidth;
+  ++Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
 }
   } else if (Contexts.back().ColonIsForRangeExpr) {
 Tok->Type = TT_RangeBasedForLoopColon;
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -243,8 +243,9 @@
   /// 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


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -515,11 +515,23 @@
 }
 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 {
+

[PATCH] D48716: [clang-format] Fix counting parameters/arguments for ObjC

2018-07-02 Thread Jacek Olesiak via Phabricator via cfe-commits 
jolesiak marked 6 inline comments as done.
jolesiak added a comment.

In https://reviews.llvm.org/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 https://reviews.llvm.org/D48716, 
https://reviews.llvm.org/D48718, https://reviews.llvm.org/D48719, 
https://reviews.llvm.org/D48720 (which are the split of 
https://reviews.llvm.org/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: https://reviews.llvm.org/D48716 -> 
https://reviews.llvm.org/D48718 -> https://reviews.llvm.org/D48719 -> 
https://reviews.llvm.org/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).

https://reviews.llvm.org/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 
https://reviews.llvm.org/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 https://reviews.llvm.org/D48719.

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


Repository:
  rC Clang

https://reviews.llvm.org/D48716



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48716: [clang-format] Fix counting parameters/arguments for ObjC

2018-07-02 Thread Jacek Olesiak via Phabricator via cfe-commits 
jolesiak marked 6 inline comments as done.
jolesiak added inline comments.



Comment at: lib/Format/FormatToken.h:247-248
+  /// If this is the first ObjC selector name in an ObjC method
+  /// definition or call, this contains the number of parts that whole selector
+  /// consist of.
   unsigned ObjCSelectorNameParts = 0;

benhamilton wrote:
> Grammar nit: that whole selector consist of -> that the whole selector 
> consists of
> 
Thanks!



Comment at: lib/Format/TokenAnnotator.cpp:519
+// FirstObjCSelectorName is set when a colon is found. This does
+// not work, however, when method has no parameters.
+// Here, we set FirstObjCSelectorName when the end of the expression is

benhamilton wrote:
> Grammar nit-pick: when method -> when the method
> 
Thanks!



Comment at: lib/Format/TokenAnnotator.cpp:520
+// not work, however, when method has no parameters.
+// Here, we set FirstObjCSelectorName when the end of the expression is
+// reached, in case it was not set already.

benhamilton wrote:
> expression -> method call
> 
Thanks!



Comment at: lib/Format/TokenAnnotator.cpp:627
   void updateParameterCount(FormatToken *Left, FormatToken *Current) {
+// For ObjC methods number of parameters is calculated differently as
+// method declarations have different structure (parameters are not inside

benhamilton wrote:
> Grammar nit-pick: methods number -> methods, the number
> 
Thanks!



Comment at: lib/Format/TokenAnnotator.cpp:628
+// For ObjC methods number of parameters is calculated differently as
+// method declarations have different structure (parameters are not inside
+// parenthesis scope).

benhamilton wrote:
> benhamilton wrote:
> > Grammar nit-picks:
> > 
> > * have different structure -> have a different structure
> > * parameters are not -> the parameters are not
> > 
> Why does parenthesis scope matter here? `updateParameterCount()` is called 
> from `parseSquare()`.
> 
> I'm not sure what the goal of this change is.
> 
Thanks!



Comment at: lib/Format/TokenAnnotator.cpp:628-629
+// For ObjC methods number of parameters is calculated differently as
+// method declarations have different structure (parameters are not inside
+// parenthesis scope).
 if (Current->is(tok::l_brace) && Current->BlockKind == BK_Block)

jolesiak wrote:
> benhamilton wrote:
> > benhamilton wrote:
> > > Grammar nit-picks:
> > > 
> > > * have different structure -> have a different structure
> > > * parameters are not -> the parameters are not
> > > 
> > Why does parenthesis scope matter here? `updateParameterCount()` is called 
> > from `parseSquare()`.
> > 
> > I'm not sure what the goal of this change is.
> > 
> Thanks!
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.


Repository:
  rC Clang

https://reviews.llvm.org/D48716



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48716: [clang-format] Fix counting parameters/arguments for ObjC

2018-06-28 Thread Ben Hamilton via Phabricator via cfe-commits 
benhamilton requested changes to this revision.
benhamilton added a comment.
This revision now requires changes to proceed.

> 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?




Comment at: lib/Format/FormatToken.h:247-248
+  /// If this is the first ObjC selector name in an ObjC method
+  /// definition or call, this contains the number of parts that whole selector
+  /// consist of.
   unsigned ObjCSelectorNameParts = 0;

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




Comment at: lib/Format/TokenAnnotator.cpp:519
+// FirstObjCSelectorName is set when a colon is found. This does
+// not work, however, when method has no parameters.
+// Here, we set FirstObjCSelectorName when the end of the expression is

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




Comment at: lib/Format/TokenAnnotator.cpp:520
+// not work, however, when method has no parameters.
+// Here, we set FirstObjCSelectorName when the end of the expression is
+// reached, in case it was not set already.

expression -> method call




Comment at: lib/Format/TokenAnnotator.cpp:627
   void updateParameterCount(FormatToken *Left, FormatToken *Current) {
+// For ObjC methods number of parameters is calculated differently as
+// method declarations have different structure (parameters are not inside

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




Comment at: lib/Format/TokenAnnotator.cpp:628
+// For ObjC methods number of parameters is calculated differently as
+// method declarations have different structure (parameters are not inside
+// parenthesis scope).

Grammar nit-picks:

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




Comment at: lib/Format/TokenAnnotator.cpp:628-629
+// For ObjC methods number of parameters is calculated differently as
+// method declarations have different structure (parameters are not inside
+// parenthesis scope).
 if (Current->is(tok::l_brace) && Current->BlockKind == BK_Block)

benhamilton wrote:
> Grammar nit-picks:
> 
> * have different structure -> have a different structure
> * parameters are not -> the parameters are not
> 
Why does parenthesis scope matter here? `updateParameterCount()` is called from 
`parseSquare()`.

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



Repository:
  rC Clang

https://reviews.llvm.org/D48716



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48716: [clang-format] Fix counting parameters/arguments for ObjC

2018-06-28 Thread Jacek Olesiak via Phabricator via cfe-commits 
jolesiak updated this revision to Diff 153298.
jolesiak added a comment.

Fix comment


Repository:
  rC Clang

https://reviews.llvm.org/D48716

Files:
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -515,11 +515,23 @@
 }
 Left->MatchingParen = CurrentToken;
 CurrentToken->MatchingParen = Left;
+// FirstObjCSelectorName is set when a colon is found. This does
+// not work, however, when method has no parameters.
+// Here, we set FirstObjCSelectorName when the end of the expression 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;
 }
@@ -539,11 +551,6 @@
  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.
@@ -617,12 +624,12 @@
   }
 
   void updateParameterCount(FormatToken *Left, FormatToken *Current) {
+// For ObjC methods number of parameters is calculated differently as
+// method declarations have different structure (parameters are not inside
+// parenthesis 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));
@@ -712,6 +719,7 @@
Contexts.back().LongestObjCSelectorName)
 Contexts.back().LongestObjCSelectorName =
 Tok->Previous->ColumnWidth;
+  ++Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
 }
   } else if (Contexts.back().ColonIsForRangeExpr) {
 Tok->Type = TT_RangeBasedForLoopColon;
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -243,8 +243,9 @@
   /// 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 whole selector
+  /// consist of.
   unsigned ObjCSelectorNameParts = 0;
 
   /// Stores the number of required fake parentheses and the


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -515,11 +515,23 @@
 }
 Left->MatchingParen = CurrentToken;
 CurrentToken->MatchingParen = Left;
+// FirstObjCSelectorName is set when a colon is found. This does
+// not work, however, when method has no parameters.
+// Here, we set FirstObjCSelectorName when the end of the expression 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 =
+

[PATCH] D48716: [clang-format] Fix counting parameters/arguments for ObjC

2018-06-28 Thread Jacek Olesiak via Phabricator via cfe-commits 
jolesiak created this revision.
Herald added a subscriber: cfe-commits.

Repository:
  rC Clang

https://reviews.llvm.org/D48716

Files:
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -515,11 +515,23 @@
 }
 Left->MatchingParen = CurrentToken;
 CurrentToken->MatchingParen = Left;
+// FirstObjCSelectorName is set when a colon is found. This does
+// not work, however, when method has no parameters.
+// Here, we set FirstObjCSelectorName when the end of the expression 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;
 }
@@ -539,11 +551,6 @@
  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.
@@ -617,12 +624,12 @@
   }
 
   void updateParameterCount(FormatToken *Left, FormatToken *Current) {
+// For ObjC methods number of parameters is calculated differently as
+// method declarations have a different structure (parameters are not 
inside
+// parenthesis 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));
@@ -712,6 +719,7 @@
Contexts.back().LongestObjCSelectorName)
 Contexts.back().LongestObjCSelectorName =
 Tok->Previous->ColumnWidth;
+  ++Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
 }
   } else if (Contexts.back().ColonIsForRangeExpr) {
 Tok->Type = TT_RangeBasedForLoopColon;
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -243,8 +243,9 @@
   /// 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 whole selector
+  /// consist of.
   unsigned ObjCSelectorNameParts = 0;
 
   /// Stores the number of required fake parentheses and the


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -515,11 +515,23 @@
 }
 Left->MatchingParen = CurrentToken;
 CurrentToken->MatchingParen = Left;
+// FirstObjCSelectorName is set when a colon is found. This does
+// not work, however, when method has no parameters.
+// Here, we set FirstObjCSelectorName when the end of the expression 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 =
+