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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
