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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.


================
Comment at: lib/Format/TokenAnnotator.cpp:329
+      return nullptr;
+    // C++17 '[[using namespace: foo, bar(baz, blech)]]'
+    bool IsUsingNamespace =
----------------
Can you  make this:

  // C++17 '[[using <namespace>: foo, bar(baz, blech)]]'

To make clear that we are not looking for kw_namespace here?


================
Comment at: lib/Format/TokenAnnotator.cpp:332
+        AttrTok->startsSequence(tok::kw_using, tok::identifier, tok::colon);
+    if (IsUsingNamespace) {
+      AttrTok = AttrTok->Next->Next->Next;
----------------
No braces.


================
Comment at: lib/Format/TokenAnnotator.cpp:336
+    auto parseCpp11Attribute = [](const FormatToken &Tok,
+                                  bool AllowNamespace) -> const FormatToken * {
+      if (!Tok.isOneOf(tok::identifier, tok::ellipsis))
----------------
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.


================
Comment at: lib/Format/TokenAnnotator.cpp:342
+        return nullptr;
+      if (AllowNamespace &&
+          AttrTok->startsSequence(tok::coloncolon, tok::identifier)) {
----------------
No braces.


================
Comment at: lib/Format/TokenAnnotator.cpp:350
+        const FormatToken *ParamToken = AttrTok->Next;
+        while (ParamToken && ParamToken->isNot(tok::r_paren))
+          ParamToken = ParamToken->Next;
----------------
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...


================
Comment at: lib/Format/TokenAnnotator.cpp:366
+      return AttrTok->Next;
+    } else {
+      return nullptr;
----------------
No braces for single statement ifs. Don't use "else" after "return".


================
Comment at: lib/Format/TokenAnnotator.cpp:396
+      while (CurrentToken != Cpp11AttributeSpecifierClosingRSquare) {
+        if (CurrentToken->is(tok::colon)) {
+          CurrentToken->Type = TT_AttributeColon;
----------------
No braces for single-statement ifs.


================
Comment at: lib/Format/TokenAnnotator.cpp:397
+        if (CurrentToken->is(tok::colon)) {
+          CurrentToken->Type = TT_AttributeColon;
+        }
----------------
What happens if you don't assign this type here? Which formatting decision is based on it?


================
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);");
----------------
If this is meant to contrast a TT_AttributeColon from a different colon, that doesn't work. "::" is it's own token type coloncolon.


Repository:
  rC Clang

https://reviews.llvm.org/D43902