sammccall added a comment.
Fantastic :-)
@rsmith and others: would appreciate feedback on giving this a feature-flag (to
decouple from LangOpts.CPlusPlus):
- whether it's OK to have one
- whether it should be cc1-only
- the name `-f[no]recovery-ast`
================
Comment at: clang/include/clang/AST/Expr.h:5920
+///
+/// RecoveryExpr is type-, value- and instantiation-dependent to take advantage
+/// of existing machinery to deal with dependent code in C++, e.g. RecoveryExpr
----------------
AIUI this should be "for now" with the goal of eliminating the use of template
dependence concepts, right?
================
Comment at: clang/include/clang/AST/Expr.h:5935
+ SourceLocation EndLoc, ArrayRef<Expr *>
SubExprs);
+ static RecoveryExpr *CreateEmpty(ASTContext &Ctx, unsigned NumStmts);
+
----------------
nit: NumStmts -> NumSubExprs?
================
Comment at: clang/lib/AST/ComputeDependence.cpp:460
+ExprDependence clang::computeDependence(RecoveryExpr *E) {
+ auto D = ExprDependence::TypeValueInstantiation | ExprDependence::Error;
+ for (auto *S : E->subExpressions())
----------------
Probably worth echoing with a FIXME: drop type+value+instantiation once Error
is sufficient to suppress checks.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:18401
+ // FIXME: use containsErrors() to suppress unwanted diags in C.
+ if (!Context.getLangOpts().CPlusPlus)
+ return ExprError();
----------------
I think we should strongly consider a LangOption with an associated flag. (e.g.
LangOptions.RecoveryAST, -f[no]recovery-ast).
If we're going to pay tho cost of letting expr creation fail, we might as well
use it
Use cases:
- Control rollout: we can check this in without (yet) flipping the flag on for
all tools at once, if desired. If we flip the default and it causes problems
for particular tools, we can turn it off for that tool rather than rolling the
whole thing back.
- turn on and off in lit tests to precisely test behavior and avoid dependence
on defaults
- allow incremental work on recovery in !CPlusPlus mode
If this makes sense to you, I'd suggest setting the default to off in this
patch (and including some tests that pass `-frecovery-ast`), and then
immediately following up with a patch that flips the default to on-for-C++.
This separation makes like easier for everyone if turning this on breaks
something.
A bunch of the updates to existing tests would be deferred until that patch.
================
Comment at: clang/lib/Sema/TreeTransform.h:9470
+ExprResult TreeTransform<Derived>::TransformRecoveryExpr(RecoveryExpr *E) {
+ return E;
+}
----------------
rsmith wrote:
> ilya-biryukov wrote:
> > rsmith wrote:
> > > We should either transform the subexpressions or just return
> > > `ExprError()` here. With this approach, we can violate AST invariants
> > > (eg, by having the same `Expr` reachable through two different code paths
> > > in the same function body, or by retaining `DeclRefExpr`s referring to
> > > declarations from the wrong context, etc).
> > Thanks, I just blindly copied what TypoExpr was doing without considering
> > the consequences here.
> >
> >
> > Added the code to rebuild `RecoveryExpr`.
> >
> > Any good way to test this?
> Hmm, testing that the old failure mode doesn't happen seems a bit tricky; any
> test for that is going to be testing second-order effects of the code under
> test, so will be fragile. But you can test that we're substituting into the
> `RecoveryExpr` easily enough: put some expression for which substitution will
> fail into a context where we'll build a `RecoveryExpr` inside a context that
> we'll `TreeTransform`. For example, maybe:
>
> ```
> template<typename T> int *p = &void(T::error); // should produce "cannot take
> address of void"
> int *q = p<int>; // should produce "'int' cannot be used prior to '::'" in
> instantiation
> ```
@hokein I don't think this test was added yet.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69330/new/
https://reviews.llvm.org/D69330
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
