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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington marked 3 inline comments as done.
erik.pilkington added inline comments.


================
Comment at: clang/include/clang/AST/DeclCXX.h:3801
+/// error.
+class UnresolvedUsingIfExistsDecl final : public NamedDecl {
+  UnresolvedUsingIfExistsDecl(DeclContext *DC, SourceLocation Loc,
----------------
aaron.ballman wrote:
> 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.)
Yeah, the answer is "yes". The AST representation for a using_if_exists declaration that failed to lookup anything would look something like this:

```
|-UsingDecl 0x7fa42b84a3a0 <t.cpp:20:1, col:9> col:9 ::X
|-UsingShadowDecl 0x7fa42b84a3f8 <col:9> col:9 implicit UnresolvedUsingIfExists 0x7fa42b84a370 'X'
```

So its still using the typical UsingDecl machinery, its just that the declaration thats being imported is different.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5273
+  namespace empty_namespace {};
+  using empty_namespace::does_not_exist __attribute__((using_if_exists)); // no error!
+
----------------
aaron.ballman wrote:
> 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.
> 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.

Yeah, agreed. Its just that Sema was failing to call ProcessDeclAttributeList for UsingDecls, so no attributes were actually getting applied in practice.

Okay, if our policy is to only parse C++-style attributes in places that the C++ grammar allows them then I agree that this would be an issue for the standards committee. 


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

https://reviews.llvm.org/D90188