rsmith added a comment.
In addition to moving the diagnostic to `DiagnoseUseOfDecl`, you may well get
better error recovery if you teach `ClassifyName` about the new kind of
declaration, and have it `DiagnoseUseOfDecl` it immediately and return
`NameClassification::Error()` -- that way, the parser knows it can't use the
kind of the name to disambiguate the parse and should use heuristics to figure
out whether a type or non-type was intended instead.
================
Comment at: clang/lib/AST/DeclBase.cpp:816
+ case UnresolvedUsingIfExists:
+ return IDNS_Type | IDNS_Member | IDNS_Ordinary;
+
----------------
Hmm. This is tricky: `IDNS_Type` isn't necessarily appropriate, but we don't
know one way or the other. Consider:
```
namespace N {
using ::stat [[clang::using_if_exists]];
struct stat;
}
```
Should we accept or reject? If the intent is that the using-declaration brings
in only a non-type name, then this should be valid, but if it's intended to
also bring in a type, then we'd have a conflict.
I suppose on balance being more restrictive (including `IDNS_Type` here) is
probably better.
You don't need `IDNS_Member` here; that's only different from `IDNS_Ordinary`
in C.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:438-439
+ NamedDecl *RealRes = *Res;
+ if (auto *ShadowD = dyn_cast<UsingShadowDecl>(*Res))
+ RealRes = ShadowD->getTargetDecl();
+ if (isa<TypeDecl>(RealRes) || isa<ObjCInterfaceDecl>(RealRes) ||
----------------
Can you use `getUnderlyingDecl` for this?
================
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))) {
----------------
================
Comment at: clang/lib/Sema/SemaDecl.cpp:500
+ // Recover with 'int'
+ T = Context.IntTy;
} else if (AllowDeducedTemplate) {
----------------
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?
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:12119
- if (HasTypenameKeyword) {
- // If we asked for a typename and got a non-type decl, error out.
- if (!R.getAsSingle<TypeDecl>()) {
- Diag(IdentLoc, diag::err_using_typename_non_type);
- for (LookupResult::iterator I = R.begin(), E = R.end(); I != E; ++I)
- Diag((*I)->getUnderlyingDecl()->getLocation(),
- diag::note_using_decl_target);
- return BuildInvalid();
+ if (!IsUsingIfExists || !R.empty()) {
+ if (HasTypenameKeyword) {
----------------
It might be simpler to create the `UnresolvedUsingIfExistsDecl` earlier (before
this `if`) and add it to the lookup result for the checks here and below. That
would avoid some of the special-case logic here.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:3146-3150
+ if (auto *EmptyD = dyn_cast<UnresolvedUsingIfExistsDecl>(D)) {
+ Diag(Loc, diag::err_use_of_empty_using_if_exists);
+ Diag(EmptyD->getLocation(), diag::note_empty_using_if_exists_here);
+ return ExprError();
+ }
----------------
I think this should instead be done by `DiagnoseUseOfDecl` / `CanUseDecl`. All
the code paths we care about already go through that.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3195-3196
+ UnresolvedUsingIfExistsDecl *D) {
+ return UnresolvedUsingIfExistsDecl::Create(
+ SemaRef.Context, Owner, D->getLocation(), D->getDeclName());
+}
----------------
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.
================
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();
+ }
----------------
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;
```
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
