[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,
----------------
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?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:554
+def note_empty_using_if_exists_here : Note<
+  "using declaration annotated with using_if_exists here">;
 
----------------
Please add single quotes around the attribute name.


================
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);
----------------
`const auto *`


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3146
   if (!VD) {
+    if (auto *EmptyD = dyn_cast<UnresolvedUsingIfExistsDecl>(D)) {
+      Diag(Loc, diag::err_use_of_empty_using_if_exists);
----------------
rsmith wrote:
> I think this should instead be done by `DiagnoseUseOfDecl` / `CanUseDecl`. All the code paths we care about already go through that.
`const auto *`


================
Comment at: clang/lib/Sema/SemaExprMember.cpp:1169
 
+  if (auto *EmptyD = dyn_cast<UnresolvedUsingIfExistsDecl>(MemberDecl)) {
+    Diag(MemberLoc, diag::err_use_of_empty_using_if_exists);
----------------
`const auto *`


================
Comment at: clang/test/SemaCXX/using-if-exists-attr.cpp:9
+
+// FIXME: Should we support the C++ spelling here?
+using NS::x [[clang::using_if_exists]]; // expected-error {{an attribute list cannot appear here}}
----------------
Mordante wrote:
> I would prefer to allow the C++ spelling, the square brackets are also supported in C when using `-fdouble-square-bracket-attributes`. (Not too useful in this case since C doesn't support namespaces.) But as mentioned before I prefer the attribute before the `using ` declaration.
I'm not certain what this FIXME means given that it is using the C++ spelling there.


================
Comment at: clang/test/SemaCXX/using-if-exists-attr.cpp:16
+template <class>
+using template_alias UIE = NS::x; // expected-warning {{'using_if_exists' attribute only applies to named declarations, types, and value declarations}}
+
----------------
erik.pilkington wrote:
> ldionne wrote:
> > I assume the attribute is ignored on these declarations? Why is this not an error instead, since it's clearly a programmer mistake?
> Yeah, its just ignored. This is how clang behaves generally with any mis-applied attribute. An error seems more appropriate to me too honestly, but that seems like a separate policy change. Maybe @aaron.ballman knows more about the history/justification for this behaviour?
It depends entirely on how you write the `Subjects` line in `Attr.td` as to whether this will warn or err. By default, we warn because most attributes should be things you can ignore. But if the semantics of the attribute suggest that an error is a better approach, you can slap an `ErrorDiag` after the subject list to cause errors if the attribute is written on the wrong subject.


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

https://reviews.llvm.org/D90188