hokein added inline comments.
================
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
----------------
sammccall wrote:
> AIUI this should be "for now" with the goal of eliminating the use of
> template dependence concepts, right?
yeah, I think so.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:18401
+ // FIXME: use containsErrors() to suppress unwanted diags in C.
+ if (!Context.getLangOpts().CPlusPlus)
+ return ExprError();
----------------
sammccall wrote:
> 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.
+1, I think this is a good idea -- particularly letting us incrementally
working on it without breaking existing tools, I was highly suspected that this
patch will fail internal tests during build copping.
I will wait for a while for other feedback @rsmith before making the actual
change.
================
Comment at: clang/lib/Sema/TreeTransform.h:9470
+ExprResult TreeTransform<Derived>::TransformRecoveryExpr(RecoveryExpr *E) {
+ return E;
+}
----------------
sammccall wrote:
> 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.
good catch, done.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69330/new/
https://reviews.llvm.org/D69330
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
