erik.pilkington added a comment. In D90188#2397441 <https://reviews.llvm.org/D90188#2397441>, @aaron.ballman wrote:
> FWIW, it'd be a bit easier if those changes were split off into their own > patch, as they're orthogonal to `using_if_exists`. Ok, I moved this part to: https://reviews.llvm.org/D91630 ================ Comment at: clang/include/clang/Basic/AttrDocs.td:5351 +the availability of those declarations is difficult or impossible to detect at +compile time with the preprocessor. + }]; ---------------- aaron.ballman wrote: > Do you think the docs should call out that using the C++ spelling on a > `using` declaration is currently a Clang extension? It's a bit of an odd > place to put that information, but I don't know of where else to mention that > this attribute is nonportable in a few different ways. Sure, I'll mention it. I'm also switching back to the GCC spelling here since we're warning on the C++ spelling by default. ================ Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:689 def err_attributes_not_allowed : Error<"an attribute list cannot appear here">; +def ext_cxx11_attr_placement : Extension< + "C++ does not allow an attribute list to appear here">, ---------------- aaron.ballman wrote: > I'm not certain what @rsmith thinks, but I think this should be `ExtWarn` and > have the `DefaultIgnore` removed -- this is a conforming extension of > something that's nonportable even in the absence of any attributes in the > attribute list. e.g., `[[]] using foo::bar;``` is not portable to parse. Sure, I'll update the docs to prefer using the GNU spelling if we're going to warn by default on this spelling. ================ Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1364 + /// An UnresolvedUsingIfExistsDecl record. + DECL_UNRESOLVED_USING_IF_EXISTS, + ---------------- aaron.ballman wrote: > Does this mean you need to bump `VERSION_MAJOR` as well? Hmm, I don't think so, it looks like the git hash is taken into account based on the comment in ASTBitCodes.h: ``` /// Version 4 of AST files also requires that the version control branch and /// revision match exactly, since there is no backward compatibility of /// AST files at this time. const unsigned VERSION_MAJOR = 11; ``` FWIW it seems like nobody else who adds new AST nodes is bumping this version. ================ Comment at: clang/test/Parser/cxx0x-attributes.cpp:160 template<typename T> using U [[]] = T; -using ns::i [[]]; // expected-error {{an attribute list cannot appear here}} +using ns::i [[]]; using [[]] ns::i; // expected-error {{an attribute list cannot appear here}} ---------------- aaron.ballman wrote: > Can you also add a test for `using foo [[]], bar []]];` syntax? Sure, I did this in the parser patch. ================ Comment at: clang/test/SemaCXX/using-if-exists-attr.cpp:9-17 +using NS::x [[clang::using_if_exists]]; // expected-warning{{C++ does not allow an attribute list to appear here}} + +[[clang::using_if_exists]] // expected-warning{{C++ does not allow an attribute list to appear here}} +using NS::not_there, NS::not_there2; + +using NS::not_there3, // expected-error {{no member named 'not_there3' in namespace 'NS'}} + NS::not_there4 [[clang::using_if_exists]]; // expected-warning{{C++ does not allow an attribute list to appear here}} ---------------- aaron.ballman wrote: > These test cases look like they belong in a `Parser` test not a `SemaCXX` > test. Sure, I moved this file over to Parser. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90188/new/ https://reviews.llvm.org/D90188 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
