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:
> 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.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:5273
+ namespace empty_namespace {};
+ using empty_namespace::does_not_exist __attribute__((using_if_exists)); //
no error!
+
----------------
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.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:440-441
+ RealRes = ShadowD->getTargetDecl();
+ if (isa<TypeDecl>(RealRes) || isa<ObjCInterfaceDecl>(RealRes) ||
+ isa<UnresolvedUsingIfExistsDecl>(RealRes) ||
+ (AllowDeducedTemplate && getAsTypeTemplateDecl(RealRes))) {
----------------
rsmith wrote:
>
Huh, didn't know `isa` did that!
================
Comment at: clang/lib/Sema/SemaDecl.cpp:500
+ // Recover with 'int'
+ T = Context.IntTy;
} else if (AllowDeducedTemplate) {
----------------
rsmith wrote:
> Mordante wrote:
> > Why do you want recover instead of returning a `nullptr`?
> I agree. Producing `IntTy` here is liable to result in follow-on diagnostics.
> It seems better to tell downstream code that you found a non-type. Can you
> remove this special case entirely?
Hmm, removing this case seems to result in worse diagnostics. For instance,
here:
```
using NS::X __attribute__((using_if_exists));
X y;
```
If we remove this check and let `getTypeName()` just return nullptr on `X`,
then the parser just emits a generic "unknown type name `X`" diagnostic. This
is the only place in that path where lookup is done on X, so ISTM like the
place where we should be checking this.
If we don't recover with some type, then the parser will sometimes assume that
this is unworkable as a type, and attempt to recover by parsing it as something
else, which can lead to duplicate diagnostics. e.g., here: `int i = X();`
I guess we could make `getTypeName()` return an `Optional<ParsedType>` that
indicated to callers that they should assume that the name was a type, even if
it can't be represented by a type node. That seems a little heavy-handed
though. WDYT?
================
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);
----------------
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.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3195-3196
+ UnresolvedUsingIfExistsDecl *D) {
+ return UnresolvedUsingIfExistsDecl::Create(
+ SemaRef.Context, Owner, D->getLocation(), D->getDeclName());
+}
----------------
rsmith wrote:
> Is this reachable? I'd expect the only way we can see these is via the list
> of shadow declarations of the `UsingDecl`, so we should never try to
> instantiate one by itself.
Yeah, this is unreachable. I added a `llvm_unreachable`.
================
Comment at: clang/lib/Sema/TreeTransform.h:14115-14123
+ NamedDecl *Target = Using->shadow_begin()->getTargetDecl();
+ if (auto *EmptyD = dyn_cast<UnresolvedUsingIfExistsDecl>(Target)) {
+ getSema().Diag(Loc, diag::err_use_of_empty_using_if_exists);
+ getSema().Diag(EmptyD->getLocation(),
+ diag::note_empty_using_if_exists_here);
+ return QualType();
+ }
----------------
rsmith wrote:
> We should `DiagnoseUseOfDecl` along this code path rather than adding an
> ad-hoc check for this one case. That would also fix another, related bug:
>
> ```
> struct A { struct __attribute__((deprecated)) B {}; };
> // warns that B is deprecated
> struct C : A { using typename A::B; B b; };
> // fails to warn that B is deprecated
> template<typename T> struct D : A { using typename A::B; B b; };
> D<A> da;
> ```
Sure, the update adds that testcase in.
================
Comment at: clang/test/SemaCXX/using-if-exists-attr.cpp:1
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only %s -verify
+
----------------
Mordante wrote:
> Does the test require C++20? If so can you use `-std=c++20` since Clang
> supports it, same for the other test.
It doesn't require C++20, I just think its good practice to always specify the
default in a testcase, since clang sometimes updates the default version and
downstream users sometimes can't take those updates, which can lead to test
breakages. New patch uses the `20` spelling, though.
================
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}}
----------------
aaron.ballman wrote:
> 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.
The C++ spelling is getting rejected with an error message, though. It seems
like this was an intentional decision:
https://github.com/llvm/llvm-project/blob/3c050a597c599cea035537b8875774dcc48922c3/clang/lib/Parse/ParseDeclCXX.cpp#L715.
I'm happy add parsing support for C++ attributes if that's what we want. I
think we could also support attributes before the using-declaration if we
wanted to (and have the attributes there apply to each using-declarator), which
I'm also happy to add. Note that the C++ grammar doesn't support attributes
anywhere on using-declarations.
================
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}}
+
----------------
aaron.ballman wrote:
> 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.
Ok sure, I agree with Louis that the error makes sense here so I added
ErrorDiag.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90188/new/
https://reviews.llvm.org/D90188
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
