sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Diagnostic changes look great!
================
Comment at: clang/lib/Sema/SemaDecl.cpp:1230
LookupResult Result(*this, Name, NameLoc, LookupOrdinaryName);
return BuildDeclarationNameExpr(SS, Result, /*ADL=*/true);
}
----------------
wonder if if the results of setting AcceptInvalidDecl here would be good/bad?
(happy with in this patch/separate one/not at all, just curious)
Also possible candidates are the calls in:
- Sema::ActOnIdExpression
- Sema::BuildQualifiedDeclarationNameExpr
- Sema::BuildPossibleImplicitMemberExpr
- BuildRecoveryCallExpr in SemaOverload
================
Comment at: clang/lib/Sema/SemaExpr.cpp:3156
if (D->isInvalidDecl())
- return true;
+ return !AcceptInvalid;
----------------
if AcceptInvalid is true here, why do we skip all the rest of the checks?
Seems like in that case there's not much point calling the function at all...
My suspicion is that if we resolve to e.g. an invalid typedef then the
diagnostic is good.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:3473
+ /*FIXME: TemplateKWLoc*/ SourceLocation(),
TemplateArgs);
+ // Many clang AST consumers assume a DeclRefExpr refers to a valid decl. We
+ // wrap a DeclRefExpr referring to an invalid decl with a dependent-type
----------------
Maybe drop the "many", which kind of implies they're incorrect to do so?
================
Comment at: clang/lib/Sema/SemaExpr.cpp:3478
+ if (VD->isInvalidDecl() && E)
+ return CreateRecoveryExpr(E->getBeginLoc(), E->getEndLoc(), {E});
+ return E;
----------------
This means we're changing the AST returned for existing
`AcceptInvalidDecl=true` callers, right?
I think this is just attemptRecovery() in SemaCXX.cpp, which is part of typo
correction. Previously we might transforming typos into DeclRefExpr to invalid,
but now we're transforming them to RecoveryExpr wrapping DeclExpr to invalid.
This seems sensible, but I'm a little concerned there are no test changes for
it. Is it possible to construct one?
I kind of expected this to work:
```
m *foo;
void z() {
goo;
}
```
but in fact we produce an opaque RecoveryExpr (with no DeclRefExpr) inside
today.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121599/new/
https://reviews.llvm.org/D121599
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
