[PATCH] D45185: [clang-format] Support lightweight Objective-C generics

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC329298: [clang-format] Support lightweight Objective-C 
generics (authored by benhamilton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D45185?vs=140968=141163#toc

Repository:
  rC Clang

https://reviews.llvm.org/D45185

Files:
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -299,6 +299,18 @@
"+ (id)init;\n"
"@end");
 
+  verifyFormat("@interface Foo  : Bar  {\n"
+   "  int _i;\n"
+   "}\n"
+   "+ (id)init;\n"
+   "@end");
+
+  verifyFormat("@interface Foo > : Xyzzy  {\n"
+   "  int _i;\n"
+   "}\n"
+   "+ (id)init;\n"
+   "@end");
+
   verifyFormat("@interface Foo (HackStuff) {\n"
"  int _i;\n"
"}\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -2122,9 +2122,13 @@
 
 void UnwrappedLineParser::parseObjCProtocolList() {
   assert(FormatTok->Tok.is(tok::less) && "'<' expected.");
-  do
+  do {
 nextToken();
-  while (!eof() && FormatTok->Tok.isNot(tok::greater));
+// Early exit in case someone forgot a close angle.
+if (FormatTok->isOneOf(tok::semi, tok::l_brace) ||
+FormatTok->Tok.isObjCAtKeyword(tok::objc_end))
+  return;
+  } while (!eof() && FormatTok->Tok.isNot(tok::greater));
   nextToken(); // Skip '>'.
 }
 
@@ -2155,7 +2159,32 @@
   nextToken();
   nextToken(); // interface name
 
-  // @interface can be followed by either a base class, or a category.
+  // @interface can be followed by a lightweight generic
+  // specialization list, then either a base class or a category.
+  if (FormatTok->Tok.is(tok::less)) {
+// Unlike protocol lists, generic parameterizations support
+// nested angles:
+//
+// @interface Foo> :
+// NSObject 
+//
+// so we need to count how many open angles we have left.
+unsigned NumOpenAngles = 1;
+do {
+  nextToken();
+  // Early exit in case someone forgot a close angle.
+  if (FormatTok->isOneOf(tok::semi, tok::l_brace) ||
+  FormatTok->Tok.isObjCAtKeyword(tok::objc_end))
+break;
+  if (FormatTok->Tok.is(tok::less))
+++NumOpenAngles;
+  else if (FormatTok->Tok.is(tok::greater)) {
+assert(NumOpenAngles > 0 && "'>' makes NumOpenAngles negative");
+--NumOpenAngles;
+  }
+} while (!eof() && NumOpenAngles != 0);
+nextToken(); // Skip '>'.
+  }
   if (FormatTok->Tok.is(tok::colon)) {
 nextToken();
 nextToken(); // base class name


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -299,6 +299,18 @@
"+ (id)init;\n"
"@end");
 
+  verifyFormat("@interface Foo  : Bar  {\n"
+   "  int _i;\n"
+   "}\n"
+   "+ (id)init;\n"
+   "@end");
+
+  verifyFormat("@interface Foo > : Xyzzy  {\n"
+   "  int _i;\n"
+   "}\n"
+   "+ (id)init;\n"
+   "@end");
+
   verifyFormat("@interface Foo (HackStuff) {\n"
"  int _i;\n"
"}\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -2122,9 +2122,13 @@
 
 void UnwrappedLineParser::parseObjCProtocolList() {
   assert(FormatTok->Tok.is(tok::less) && "'<' expected.");
-  do
+  do {
 nextToken();
-  while (!eof() && FormatTok->Tok.isNot(tok::greater));
+// Early exit in case someone forgot a close angle.
+if (FormatTok->isOneOf(tok::semi, tok::l_brace) ||
+FormatTok->Tok.isObjCAtKeyword(tok::objc_end))
+  return;
+  } while (!eof() && FormatTok->Tok.isNot(tok::greater));
   nextToken(); // Skip '>'.
 }
 
@@ -2155,7 +2159,32 @@
   nextToken();
   nextToken(); // interface name
 
-  // @interface can be followed by either a base class, or a category.
+  // @interface can be followed by a lightweight generic
+  // specialization list, then either a base class or a category.
+  if (FormatTok->Tok.is(tok::less)) {
+// Unlike protocol lists, generic parameterizations support
+// nested angles:
+//
+// @interface Foo> :
+// NSObject 
+//
+// so we need to count how many open angles we have left.
+unsigned NumOpenAngles = 

[PATCH] D45185: [clang-format] Support lightweight Objective-C generics

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good, thank you!


Repository:
  rC Clang

https://reviews.llvm.org/D45185



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

[PATCH] D45185: [clang-format] Support lightweight Objective-C generics

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton updated this revision to Diff 140968.
benhamilton marked 3 inline comments as done.
benhamilton added a comment.

- @djasper fixes


Repository:
  rC Clang

https://reviews.llvm.org/D45185

Files:
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -298,6 +298,18 @@
"+ (id)init;\n"
"@end");
 
+  verifyFormat("@interface Foo  : Bar  {\n"
+   "  int _i;\n"
+   "}\n"
+   "+ (id)init;\n"
+   "@end");
+
+  verifyFormat("@interface Foo > : Xyzzy  {\n"
+   "  int _i;\n"
+   "}\n"
+   "+ (id)init;\n"
+   "@end");
+
   verifyFormat("@interface Foo (HackStuff) {\n"
"  int _i;\n"
"}\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -2122,9 +2122,13 @@
 
 void UnwrappedLineParser::parseObjCProtocolList() {
   assert(FormatTok->Tok.is(tok::less) && "'<' expected.");
-  do
+  do {
 nextToken();
-  while (!eof() && FormatTok->Tok.isNot(tok::greater));
+// Early exit in case someone forgot a close angle.
+if (FormatTok->isOneOf(tok::semi, tok::l_brace) ||
+FormatTok->Tok.isObjCAtKeyword(tok::objc_end))
+  return;
+  } while (!eof() && FormatTok->Tok.isNot(tok::greater));
   nextToken(); // Skip '>'.
 }
 
@@ -2155,7 +2159,32 @@
   nextToken();
   nextToken(); // interface name
 
-  // @interface can be followed by either a base class, or a category.
+  // @interface can be followed by a lightweight generic
+  // specialization list, then either a base class or a category.
+  if (FormatTok->Tok.is(tok::less)) {
+// Unlike protocol lists, generic parameterizations support
+// nested angles:
+//
+// @interface Foo> :
+// NSObject 
+//
+// so we need to count how many open angles we have left.
+unsigned NumOpenAngles = 1;
+do {
+  nextToken();
+  // Early exit in case someone forgot a close angle.
+  if (FormatTok->isOneOf(tok::semi, tok::l_brace) ||
+  FormatTok->Tok.isObjCAtKeyword(tok::objc_end))
+break;
+  if (FormatTok->Tok.is(tok::less))
+++NumOpenAngles;
+  else if (FormatTok->Tok.is(tok::greater)) {
+assert(NumOpenAngles > 0 && "'>' makes NumOpenAngles negative");
+--NumOpenAngles;
+  }
+} while (!eof() && NumOpenAngles != 0);
+nextToken(); // Skip '>'.
+  }
   if (FormatTok->Tok.is(tok::colon)) {
 nextToken();
 nextToken(); // base class name


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -298,6 +298,18 @@
"+ (id)init;\n"
"@end");
 
+  verifyFormat("@interface Foo  : Bar  {\n"
+   "  int _i;\n"
+   "}\n"
+   "+ (id)init;\n"
+   "@end");
+
+  verifyFormat("@interface Foo > : Xyzzy  {\n"
+   "  int _i;\n"
+   "}\n"
+   "+ (id)init;\n"
+   "@end");
+
   verifyFormat("@interface Foo (HackStuff) {\n"
"  int _i;\n"
"}\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -2122,9 +2122,13 @@
 
 void UnwrappedLineParser::parseObjCProtocolList() {
   assert(FormatTok->Tok.is(tok::less) && "'<' expected.");
-  do
+  do {
 nextToken();
-  while (!eof() && FormatTok->Tok.isNot(tok::greater));
+// Early exit in case someone forgot a close angle.
+if (FormatTok->isOneOf(tok::semi, tok::l_brace) ||
+FormatTok->Tok.isObjCAtKeyword(tok::objc_end))
+  return;
+  } while (!eof() && FormatTok->Tok.isNot(tok::greater));
   nextToken(); // Skip '>'.
 }
 
@@ -2155,7 +2159,32 @@
   nextToken();
   nextToken(); // interface name
 
-  // @interface can be followed by either a base class, or a category.
+  // @interface can be followed by a lightweight generic
+  // specialization list, then either a base class or a category.
+  if (FormatTok->Tok.is(tok::less)) {
+// Unlike protocol lists, generic parameterizations support
+// nested angles:
+//
+// @interface Foo> :
+// NSObject 
+//
+// so we need to count how many open angles we have left.
+unsigned NumOpenAngles = 1;
+do {
+  nextToken();
+  // Early exit in case someone forgot a close angle.
+  if (FormatTok->isOneOf(tok::semi, 

[PATCH] D45185: [clang-format] Support lightweight Objective-C generics

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

Thanks, fixed!




Comment at: lib/Format/UnwrappedLineParser.cpp:2135
+nextToken();
+if (FormatTok->Tok.is(tok::less))
+  NumOpenAngles++;

djasper wrote:
> The UnwrappedLineParser is very much about error recovery. Implemented like 
> this, it will consume the rest of the file if someone forgets to add the 
> closing ">", which can very easily happen when formatting in an editor while 
> coding.
> 
> Are there things (e.g. semicolons and braces) that clearly point to this 
> having happened so that clang-format can recover?
Good call. I had modeled this after 
`UnwrappedLineParser::parseObjCProtocolList()`, which has the same issue.

Fixed both that method and this one to exit early on semicolon, l_brace, and 
`@end`.




Comment at: lib/Format/UnwrappedLineParser.cpp:2136
+if (FormatTok->Tok.is(tok::less))
+  NumOpenAngles++;
+else if (FormatTok->Tok.is(tok::greater))

djasper wrote:
> I think we generally prefer:
> 
>   ++NumOpenAngles;
Fixed.



Comment at: lib/Format/UnwrappedLineParser.cpp:2138
+else if (FormatTok->Tok.is(tok::greater))
+  NumOpenAngles--;
+  } while (!eof() && NumOpenAngles != 0);

djasper wrote:
> I am slightly worried that this loop might be getting more complicated and it 
> will be harder to determine that NumOpenAngles cannot be 0 here. Might be 
> worth adding an assert. But might be overkill.
Assert added.



Comment at: lib/Format/UnwrappedLineParser.cpp:2181
+  if (FormatTok->Tok.is(tok::less))
+parseObjCLightweightGenericList();
+

djasper wrote:
> Are there more places where we might want to parse a lightweight generic 
> list? If this is going to remain they only instance, I think I'd prefer a 
> local lambda or just inlining the code. But up to you.
It's just the one place, so inlined the code.

Protocol lists can appear both after `@interface` and `@protocol`, but 
lightweight generics are only for `@interface`. I confirmed this with a quick 
test — changing `@protocol` to `@interface` in the example below allows it to 
compile:

```
% cat test.m
#import 

@protocol Foo  : NSObject
- (id)foo:(KeyType)aKey;
@end

lang -c test.m  
cattest.m:3:16: error: cannot find protocol declaration for 'KeyType'
@protocol Foo  : NSObject
   ^
test.m:3:25: error: expected identifier or '('
@protocol Foo  : NSObject
^
test.m:4:12: error: expected a type
- (id)foo:(KeyType)aKey;
   ^
```



Comment at: lib/Format/UnwrappedLineParser.cpp:2182
+parseObjCLightweightGenericList();
+
   if (FormatTok->Tok.is(tok::colon)) {

djasper wrote:
> nitpick: As the comment refers to this following block as well, I'd remove 
> the empty line.
Done.


Repository:
  rC Clang

https://reviews.llvm.org/D45185



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

[PATCH] D45185: [clang-format] Support lightweight Objective-C generics

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:2135
+nextToken();
+if (FormatTok->Tok.is(tok::less))
+  NumOpenAngles++;

The UnwrappedLineParser is very much about error recovery. Implemented like 
this, it will consume the rest of the file if someone forgets to add the 
closing ">", which can very easily happen when formatting in an editor while 
coding.

Are there things (e.g. semicolons and braces) that clearly point to this having 
happened so that clang-format can recover?



Comment at: lib/Format/UnwrappedLineParser.cpp:2136
+if (FormatTok->Tok.is(tok::less))
+  NumOpenAngles++;
+else if (FormatTok->Tok.is(tok::greater))

I think we generally prefer:

  ++NumOpenAngles;



Comment at: lib/Format/UnwrappedLineParser.cpp:2138
+else if (FormatTok->Tok.is(tok::greater))
+  NumOpenAngles--;
+  } while (!eof() && NumOpenAngles != 0);

I am slightly worried that this loop might be getting more complicated and it 
will be harder to determine that NumOpenAngles cannot be 0 here. Might be worth 
adding an assert. But might be overkill.



Comment at: lib/Format/UnwrappedLineParser.cpp:2181
+  if (FormatTok->Tok.is(tok::less))
+parseObjCLightweightGenericList();
+

Are there more places where we might want to parse a lightweight generic list? 
If this is going to remain they only instance, I think I'd prefer a local 
lambda or just inlining the code. But up to you.



Comment at: lib/Format/UnwrappedLineParser.cpp:2182
+parseObjCLightweightGenericList();
+
   if (FormatTok->Tok.is(tok::colon)) {

nitpick: As the comment refers to this following block as well, I'd remove the 
empty line.


Repository:
  rC Clang

https://reviews.llvm.org/D45185



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

[PATCH] D45185: [clang-format] Support lightweight Objective-C generics

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton created this revision.
benhamilton added reviewers: djasper, jolesiak.
Herald added subscribers: cfe-commits, klimek.

Previously, `clang-format` didn't understand lightweight
Objective-C generics, which have the form:

  @interface Foo  ...

The lightweight generic specifier list appears before the base
class, if present, but because it starts with < like the protocol
specifier list, `UnwrappedLineParser` was getting confused and
failed to parse interfaces with both generics and protocol lists:

  @interface Foo  : NSObject 

Since the parsed line would be incomplete, the format result
would be very confused (e.g., https://bugs.llvm.org/show_bug.cgi?id=24381).

This fixes the issue by explicitly parsing the ObjC lightweight
generic conformance list, so the line is fully parsed.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=24381

Test Plan: New tests added. Ran tests with:

  % make -j16 FormatTests && ./tools/clang/unittests/Format/FormatTests


Repository:
  rC Clang

https://reviews.llvm.org/D45185

Files:
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -298,6 +298,18 @@
"+ (id)init;\n"
"@end");
 
+  verifyFormat("@interface Foo  : Bar  {\n"
+   "  int _i;\n"
+   "}\n"
+   "+ (id)init;\n"
+   "@end");
+
+  verifyFormat("@interface Foo > : Xyzzy  {\n"
+   "  int _i;\n"
+   "}\n"
+   "+ (id)init;\n"
+   "@end");
+
   verifyFormat("@interface Foo (HackStuff) {\n"
"  int _i;\n"
"}\n"
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -116,6 +116,7 @@
   // parses the record as a child block, i.e. if the class declaration is an
   // expression.
   void parseRecord(bool ParseAsExpr = false);
+  void parseObjCLightweightGenericList();
   void parseObjCProtocolList();
   void parseObjCUntilAtEnd();
   void parseObjCInterfaceOrImplementation();
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -2120,6 +2120,26 @@
   // "} n, m;" will end up in one unwrapped line.
 }
 
+void UnwrappedLineParser::parseObjCLightweightGenericList() {
+  assert(FormatTok->Tok.is(tok::less) && "'<' expected.");
+  // Unlike protocol lists, generic parameterizations support
+  // nested angles:
+  //
+  // @interface Foo> :
+  // NSObject 
+  //
+  // so we need to count how many open angles we have left.
+  unsigned NumOpenAngles = 1;
+  do {
+nextToken();
+if (FormatTok->Tok.is(tok::less))
+  NumOpenAngles++;
+else if (FormatTok->Tok.is(tok::greater))
+  NumOpenAngles--;
+  } while (!eof() && NumOpenAngles != 0);
+  nextToken(); // Skip '>'.
+}
+
 void UnwrappedLineParser::parseObjCProtocolList() {
   assert(FormatTok->Tok.is(tok::less) && "'<' expected.");
   do
@@ -2155,7 +2175,11 @@
   nextToken();
   nextToken(); // interface name
 
-  // @interface can be followed by either a base class, or a category.
+  // @interface can be followed by a lightweight generic
+  // specialization list, then either a base class or a category.
+  if (FormatTok->Tok.is(tok::less))
+parseObjCLightweightGenericList();
+
   if (FormatTok->Tok.is(tok::colon)) {
 nextToken();
 nextToken(); // base class name


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -298,6 +298,18 @@
"+ (id)init;\n"
"@end");
 
+  verifyFormat("@interface Foo  : Bar  {\n"
+   "  int _i;\n"
+   "}\n"
+   "+ (id)init;\n"
+   "@end");
+
+  verifyFormat("@interface Foo > : Xyzzy  {\n"
+   "  int _i;\n"
+   "}\n"
+   "+ (id)init;\n"
+   "@end");
+
   verifyFormat("@interface Foo (HackStuff) {\n"
"  int _i;\n"
"}\n"
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -116,6 +116,7 @@
   // parses the record as a child block, i.e. if the class declaration is an
   // expression.
   void parseRecord(bool ParseAsExpr = false);
+  void