rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land.
In D69330#1752089 <https://reviews.llvm.org/D69330#1752089>, @ilya-biryukov wrote: > In D69330#1746137 <https://reviews.llvm.org/D69330#1746137>, @rsmith wrote: > > > I would prefer that we have dedicated tests for them rather than scattering > > the tests throughout the existing test suite (for example, consider adding > > `-Wno-unused` to the tests producing "result unused" warnings and adding a > > dedicated test). > > > Most updated tests (including those with more "result unused" warnings) are > actually not intended to test recovery expressions, they just happen to > produce different results now and need to be updated. > The only new dedicated tests here are `clang/test/AST/ast-dump-recovery.cpp` > and `clang/test/Index/getcursor-recovery.cpp`. > > Could technically move them into the same directory, but wanted to make sure > I got your point first. Could you elaborate on what testing strategy you'd > prefer? For the tests where you added `expected-warning {{unused}}` that are testing things other than parser recovery, I would instead add `-Wno-unused` or a cast to `void`. `expect`s unrelated to the intent of tests make them less readable and more fragile. I think it's fine to use `test/AST/ast-dump-recovery.cpp` as the primary test for the various forms of recovery that you added here. I've not thought of anything better, at least :) ================ Comment at: clang/lib/Sema/TreeTransform.h:9470 +ExprResult TreeTransform<Derived>::TransformRecoveryExpr(RecoveryExpr *E) { + return E; +} ---------------- 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 ``` 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
