[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Ben Hamilton via Phabricator via cfe-commits]

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