[PATCH] D45185: [clang-format] Support lightweight Objective-C generics
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&id=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 = 1; +do { + nextToken(); + // Early exit in case someone forg
[PATCH] D45185: [clang-format] Support lightweight Objective-C generics
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
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, tok::l_brace) || + FormatTok->Tok.isObjCAtKeyword(tok::objc_end)) +
[PATCH] D45185: [clang-format] Support lightweight Objective-C generics
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
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
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 parseObjCLightweightGenericList(); void parseObjCProtocolList(); void parseObjCUntilAtEnd(); void parseObjCInterfaceOrImplementation(); Index: lib/Format/UnwrappedLineParser.cpp