[PATCH] D90188: Add support for attribute 'using_if_exists'

[Erik Pilkington via Phabricator via cfe-commits]

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