usaxena95 added inline comments.
================ Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1375 + if (Trap.hasErrorOccurred()) + TransReq.getAs<RequiresExpr>()->setSatisfied(false); + } ---------------- ilya-biryukov wrote: > ilya-biryukov wrote: > > `TransReq` may be `ExprError` and this will cause a crash. Worth adding a > > test too. > Could you please add `assert(!TransReq || *TransReq != E)`. > The common optimization in TreeTransform is to avoid rebuilding the AST nodes > if nothing changes. There is no optimization like this for `RequireExpr` > right now, but it would not be unexpected if this gets implemented in the > future. > > In those situations, the current code can potentially change value of > `isSatisfied` for an existing expression rather than for a newly created, > which seems like asking for trouble. It would be nice to give an early > warning to implementors of this optimization that they should think how to > handle this case. Thanks for spotting. Adding a test with invalid requirement which caused a crash. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140547/new/ https://reviews.llvm.org/D140547 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
