[PATCH] D127487: [Sema] Fix assertion failure when instantiating requires expression

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2039
     TransConstraint = TransformExpr(Req->getConstraintExpr());
+    if (!TransConstraint.isInvalid()) {
+      bool CheckSucceeded =
----------------
erichkeane wrote:
> I think I'd rather collapse this into SOMETHING like:
> 
> ``` assert((TransConstraint.isInvalid() || (SemaRef.CheckConstraintExpr(TransConstraint.get() || Trap.hasErrorOccurred()) && "...");```
> 
> I think thats right?  
`CheckConstraintExpression` has to always run, even when assertions are disabled. It does checks for C++ semantics, e.g. checks that the type is bool.

The assertion is only there to check that whenever the function fails, `Trap` captures the actual diagnostic so the code below that relies on `hasErrorOccurred` ends up in a valid state.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2048
+    // Use version of CheckConstraintSatisfaction that does no substitutions.
+    if (!TransConstraint.isInvalid() &&
+        !TransConstraint.get()->isInstantiationDependent() &&
----------------
erichkeane wrote:
> Same sorta thing here, if our check is only used for an assert, we should call it in the assert.
See the comment above, the call to `CheckConstraintSatisfaction` is also to implement semantic C++ checks, not solely for assertion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127487/new/

https://reviews.llvm.org/D127487