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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I'm not quite understanding this yet, so I'll have to take another look early next week.  However, I AM intending to get https://reviews.llvm.org/D126907 committed in the next week or so.  Could you perhaps see how it interacts with that?  Its a sizable, multi-month project that I'd like to make sure doesn't get stuck in a rebase-loop again.



================
Comment at: clang/lib/Sema/SemaConcept.cpp:352
+      [this](const Expr *AtomicExpr) -> ExprResult {
+        // We only do this to immitate lvalue-to-rvalue conversion.
+        return PerformContextuallyConvertToBool(const_cast<Expr*>(AtomicExpr));
----------------
Can you explain this more?  How does this work, and why don't we do that directly instead?


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2042
+        !SemaRef.CheckConstraintExpression(TransConstraint.get())) {
+      assert(Trap.hasErrorOccurred() && "CheckConstraintExpression failed, but "
+                                        "did not produce a SFINAE error");
----------------
This branch ends up being empty if asserts are off.  Also, it results in CheckConstraintExpression happening 2x, which ends up being more expensive after https://reviews.llvm.org/D126907


================
Comment at: clang/test/SemaTemplate/concepts-PR54629.cpp:10
+int main() {
+  A<int> a;
+}
----------------
Simply 'doesn't crash' isn't quite enough for a test here, I would like to see some level of confirmation which of the versions of "A" get selected here.  So perhaps `A<double>{}.some_func();` call that wouldn't be valid/etc.  And perhaps a situation where both instances  have a constraint and and we diagnose why it doesn't fit?





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127487