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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
