[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 <Foundation/Foundation.h>

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

lang -c test.m                      
cattest.m:3:16: error: cannot find protocol declaration for 'KeyType'
@protocol Foo <KeyType> : NSObject
               ^
test.m:3:25: error: expected identifier or '('
@protocol Foo <KeyType> : 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