[PATCH] D45185: [clang-format] Support lightweight Objective-C generics
Daniel Jasper via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 3 23:44:21 PDT 2018
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
More information about the cfe-commits
mailing list