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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits