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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/DeclCXX.h:3801
+/// error.
+class UnresolvedUsingIfExistsDecl final : public NamedDecl {
+  UnresolvedUsingIfExistsDecl(DeclContext *DC, SourceLocation Loc,
----------------
erik.pilkington wrote:
> aaron.ballman wrote:
> > Why is this inheriting from a `NamedDecl` rather than a `UsingDecl`? Given that this is a type of using declaration, I guess I would have expected it to appear as such in the AST hierarchy. For instance, should people using AST matchers be able to match one of these as a using declaration or are they so different semantically that they need to be sibling AST nodes?
> This node isn't a kind of using declaration, it is a declaration that gets inserted into the scope via a usual UsingDecl & UsingShadowDecl mechanism that Sema knows to error out on if it is ever used. So presumably existing AST users would still recognize that this is a UsingDecl that adds a single declaration into the current context, but wouldn't really know anything about that declaration. I updated the doc comment above to make that more clear.
So given code like this:
```
using empty_namespace::does_not_exist __attribute__((using_if_exists)); 
```
would you expect this AST matcher to match or not?
```
usingDecl()
```
(I hope the answer is "yes" because otherwise the behavior is rather inexplicable to me.)


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5273
+  namespace empty_namespace {};
+  using empty_namespace::does_not_exist __attribute__((using_if_exists)); // no error!
+
----------------
erik.pilkington wrote:
> Mordante wrote:
> > Mordante wrote:
> > > Can you add an example using `[[clang::using_if_exists]]` or use that instead of this attribute?
> > Why is the attribute placed here? I would expect the attribute before the declaration `[[clang::using_if_exists]] using empty_namespace::does_not_exist;`
> The attribute is written like that because clang rejects C++ style attributes on using declarations, and only accepts attributes in that position. I think this is the first attribute we actually support on using-declarations, so maybe we should consider supporting it.
> I think this is the first attribute we actually support on using-declarations, so maybe we should consider supporting it.

This isn't the first attribute we *should* be supporting on using declarations. Any attribute that appertains to a `NamedDecl` should apply as should the annotate attribute.

However:
```
[[clang::whatever]] using foo::bar; // Correct to reject, #1
using foo::bar [[clang::whatever]]; // Correct to reject, #2
```
#1 is rejected because it's a declaration-statement and those cannot have a leading attribute-specifier-seq (http://eel.is/c++draft/stmt.stmt#stmt.pre-1). #2 is rejected because the using-declaration cannot have a trailing attribute-specifier-seq (http://eel.is/c++draft/namespace.udecl#nt:using-declaration).

This seems like a case where we may want to explore an extension to C++ that we propose to WG21.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:1177
 
+  if (auto *EmptyD = dyn_cast<UnresolvedUsingIfExistsDecl>(FirstDecl)) {
+    Diag(NameLoc, diag::err_use_of_empty_using_if_exists);
----------------
erik.pilkington wrote:
> aaron.ballman wrote:
> > `const auto *`
> This would lead to a bit of a `const`-goosechase in DiagnoseUseOfDecl. I thought we generally weren't too interested in `const` on AST nodes since they're assumed to be immutable anyways, so it doesn't really show much intent.
Ah, then don't bother with the goose chase. We don't require `const` correctness because of these kinds of goose chases, but we usually strive for `const` correctness when it's cheap to do so.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90188/new/

https://reviews.llvm.org/D90188