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

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

Thanks very much for the code review. I fixed those issues.



================
Comment at: lib/Format/TokenAnnotator.cpp:323
 
+  const FormatToken *parseCpp11Attribute(const FormatToken *Tok,
+                                         bool NamespaceAllowed) {
----------------
jolesiak wrote:
> I feel like it would be clearer (and more consistent) if we used const& here and just make a copy inside function body.
> I see no benefit of passing nullptr based on usage presented in this patch.
Fixed.


================
Comment at: lib/Format/TokenAnnotator.cpp:325
+                                         bool NamespaceAllowed) {
+    if (!Tok || !Tok->isOneOf(tok::identifier, tok::ellipsis)) return Tok;
+    Tok = Tok->Next;
----------------
jolesiak wrote:
> For consistency reasons I wouldn't use one line ifs. Let's keep existing format with line break and without braces.
Fixed.  (My mistake was using `git-clang-format` without passing `--style llvm` — for some reason that's applying a different style.)


================
Comment at: lib/Format/TokenAnnotator.cpp:337
+        ParamToken = ParamToken->Next;
+      if (!ParamToken || ParamToken->isNot(tok::r_paren)) return nullptr;
+      Tok = ParamToken->Next;
----------------
jolesiak wrote:
> Is second part of this condition necessary?
Not needed. Removed.


================
Comment at: lib/Format/TokenAnnotator.cpp:345
+  // never a valid Objective-C or Objective-C++ method invocation.
+  bool parseCpp11AttributeSpecifier(FormatToken *Tok) {
+    if (!Style.isCpp()) return false;
----------------
jolesiak wrote:
> Pointer should be to const type, but same as above, I feel like const& would be a better choice.
Fixed.


================
Comment at: lib/Format/TokenAnnotator.cpp:351
+    if (!AttributeToken) return false;
+    // C++17 '[[using namespace: foo, bar(baz, blech)]]'
+    if (AttributeToken->startsSequence(tok::kw_using, tok::identifier,
----------------
jolesiak wrote:
> How about:
> 
> ```
>     bool NamespaceAllowed;
>     if (AttributeToken->startsSequence(tok::kw_using, tok::identifier,
>                                        tok::colon)) {
>       // C++17 '[[using namespace: foo, bar(baz, blech)]]'
>       AttributeToken = AttributeToken->Next->Next->Next;
>       NamespaceAllowed = false;
>     } else {
>       // C++11 '[[namespace::foo, namespace::bar(baz, blech)]]'
>       NamespaceAllowed = true;
>     }
>     while (AttributeToken) {
>       AttributeToken =
>           parseCpp11Attribute(AttributeToken, NamespaceAllowed);
>       if (!AttributeToken || AttributeToken->isNot(tok::comma)) break;
>       AttributeToken = AttributeToken->Next;
>     }
> ```
Fixed.


Repository:
  rC Clang

https://reviews.llvm.org/D43902