benhamilton added a comment.
Great, I refactored it to avoid parsing the matching parens, so there's no more
danger of O(N^2) parsing behavior.
I had to add a new `InCpp11AttributeSpecifier` boolean to `Context` so when
parsing the inner `[]` we didn't treat it as an ObjC message send or an array
subscript.
================
Comment at: lib/Format/TokenAnnotator.cpp:329
+ return nullptr;
+ // C++17 '[[using namespace: foo, bar(baz, blech)]]'
+ bool IsUsingNamespace =
----------------
djasper wrote:
> Can you make this:
>
> // C++17 '[[using <namespace>: foo, bar(baz, blech)]]'
>
> To make clear that we are not looking for kw_namespace here?
Ah, good call. Done.
================
Comment at: lib/Format/TokenAnnotator.cpp:332
+ AttrTok->startsSequence(tok::kw_using, tok::identifier, tok::colon);
+ if (IsUsingNamespace) {
+ AttrTok = AttrTok->Next->Next->Next;
----------------
djasper wrote:
> No braces.
Done.
================
Comment at: lib/Format/TokenAnnotator.cpp:336
+ auto parseCpp11Attribute = [](const FormatToken &Tok,
+ bool AllowNamespace) -> const FormatToken * {
+ if (!Tok.isOneOf(tok::identifier, tok::ellipsis))
----------------
djasper wrote:
> Do you actually need to put the return type here? I would have thought that
> it can be deduced as you pass back a const FormatToken* from a codepath and
> nullptr from all the others.
Looks like we don't. Fixed.
================
Comment at: lib/Format/TokenAnnotator.cpp:342
+ return nullptr;
+ if (AllowNamespace &&
+ AttrTok->startsSequence(tok::coloncolon, tok::identifier)) {
----------------
djasper wrote:
> No braces.
Done.
================
Comment at: lib/Format/TokenAnnotator.cpp:350
+ const FormatToken *ParamToken = AttrTok->Next;
+ while (ParamToken && ParamToken->isNot(tok::r_paren))
+ ParamToken = ParamToken->Next;
----------------
djasper wrote:
> Sorry that I have missed this before, I thought this was in a different file.
> Can you try to avoid iterating trying to count or match parentheses inside
> any of parseSquare/parseParen/parseAngle. We avoided that AFAICT for
> everything else and I don't think it's necessary here. Especially as you are
> not actually moving the token position forward, it's too easy to create a
> quadratic algorithm here.
>
> Also: Do you actually have a test case for the the parentheses case? This
> thing could use a lot more comments...
Fixed. There is a test case for the parentheses case.
I definitely see from your other reviews that it's important to structure code
the way you describe.
In this scenario, we technically have to deal with all of these
(http://en.cppreference.com/w/cpp/language/attributes):
```
void f() {
int y[3];
int i [[cats::meow([[]])]]; // OK
int i [[cats::meow(foo:bar)]]; // OK
int i [[cats::meow(foo::bar)]]; // OK
}
```
I was able to fudge it and assume nobody will actually use attribute parameters
containing colons in the arguments, which allowed me to avoid looking for
closing parens.
================
Comment at: lib/Format/TokenAnnotator.cpp:366
+ return AttrTok->Next;
+ } else {
+ return nullptr;
----------------
djasper wrote:
> No braces for single statement ifs. Don't use "else" after "return".
Fixed.
================
Comment at: lib/Format/TokenAnnotator.cpp:396
+ while (CurrentToken != Cpp11AttributeSpecifierClosingRSquare) {
+ if (CurrentToken->is(tok::colon)) {
+ CurrentToken->Type = TT_AttributeColon;
----------------
djasper wrote:
> No braces for single-statement ifs.
Fixed.
================
Comment at: lib/Format/TokenAnnotator.cpp:397
+ if (CurrentToken->is(tok::colon)) {
+ CurrentToken->Type = TT_AttributeColon;
+ }
----------------
djasper wrote:
> What happens if you don't assign this type here? Which formatting decision is
> based on it?
Added a comment to explain:
// Remember that this is a [[using ns: foo]] C++ attribute, so we
don't
// add a space before the colon (unlike other colons).
There is a formatting test for this.
================
Comment at: unittests/Format/FormatTest.cpp:6064
+ verifyFormat("SomeType s [[unused]] (InitValue);");
+ verifyFormat("SomeType s [[gnu::unused]] (InitValue);");
+ verifyFormat("SomeType s [[using gnu: unused]] (InitValue);");
----------------
djasper wrote:
> If this is meant to contrast a TT_AttributeColon from a different colon, that
> doesn't work. "::" is it's own token type coloncolon.
It's not, I just wanted to make sure we had a test with a namespace.
Repository:
rC Clang
https://reviews.llvm.org/D43902
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
