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

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.


================
Comment at: lib/Format/TokenAnnotator.cpp:323
 
+  const FormatToken *parseCpp11Attribute(const FormatToken &Tok,
+                                         bool NamespaceAllowed) {
----------------
krasimir wrote:
> Please inline this into the other function: it's used only once and its name and arguments aren't clear outside of that context.
OK, made this a lambda.


================
Comment at: lib/Format/TokenAnnotator.cpp:352
+      return false;
+    if (!Tok.startsSequence(tok::l_square, tok::l_square))
+      return false;
----------------
djasper wrote:
> Just join this if with the previous one.
Fixed.


================
Comment at: lib/Format/TokenAnnotator.cpp:357
+      return false;
+    bool NamespaceAllowed;
+    // C++17 '[[using namespace: foo, bar(baz, blech)]]'
----------------
djasper wrote:
> Replace lines 355-365 with:
> 
>   bool IsUsingNamespace = AttrTok && AttrTok->startsSequence( ... );
>   if (IsUsingNamespace)
>     AttrTok = AttrTok->Next->Next->Next;
Fixed.


================
Comment at: lib/Format/TokenAnnotator.cpp:374
+      return false;
+    return AttrTok->startsSequence(tok::r_square, tok::r_square);
+  }
----------------
djasper wrote:
> Why not replace these three lines with:
> 
>   return AttrTok && AttrTok->startsSequence(..);
Fixed.


================
Comment at: lib/Format/TokenAnnotator.cpp:400
+      next();
+      return true;
+    }
----------------
djasper wrote:
> This seems weird. I think if we return true, we should have consumed everything up to the closing r_square.
That's super useful feedback, I wasn't sure what the contract of this method was.

Fixed to consume everything including the closing r_square. (I actually had to consume that too, or parsing was left with an un-parsed value and parsing would fail.)


================
Comment at: unittests/Format/FormatTest.cpp:12083
 
+TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[noreturn]];"));
----------------
krasimir wrote:
> djasper wrote:
> > You are not adding any test (AFAICT) that tests that you have correctly set TT_AttributeSpecifier or that you are making any formatting decisions based on it. If you can't test it, remove that part of this patch.
> This will also require adding tests about not messing up the formatting inside/around attribute specifiers too much. At least take these examples and copy them to the C++/ObjC formatting unittests.
Added formatting tests and logic to use this.

Renamed to `TT_AttributeSquare` to match `TT_AttributeParen`.



================
Comment at: unittests/Format/FormatTest.cpp:12083
 
+TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[noreturn]];"));
----------------
benhamilton wrote:
> krasimir wrote:
> > djasper wrote:
> > > You are not adding any test (AFAICT) that tests that you have correctly set TT_AttributeSpecifier or that you are making any formatting decisions based on it. If you can't test it, remove that part of this patch.
> > This will also require adding tests about not messing up the formatting inside/around attribute specifiers too much. At least take these examples and copy them to the C++/ObjC formatting unittests.
> Added formatting tests and logic to use this.
> 
> Renamed to `TT_AttributeSquare` to match `TT_AttributeParen`.
> 
I don't think we have any tests for square-bracket attribute specifier formatting (only `__attribute__((foo))`) today. My guess is it's pretty broken.

I've added more tests.


================
Comment at: unittests/Format/FormatTest.cpp:6068-6069
+  verifyFormat("void f() [[deprecated(\"so sorry\")]];");
+  verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+               "    [[unused]] aaaaaaaaaaaaaaaaaaaaaaa(int i);");
+}
----------------
I couldn't figure out why this breaks before the `[` in `[[unused]]`, when the identical test using `__attribute__((unused))` above does *not* break before the `__attribute__`.

I ran with `-debug` and the two seem fairly similar. Can anyone help shed light on this?

## With `__attribute__((unused))`

P8067

## With `[[unused]]`

P8068


Repository:
  rC Clang

https://reviews.llvm.org/D43902