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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D127487#3573180 <https://reviews.llvm.org/D127487#3573180>, @erichkeane wrote:

> 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.

Sure, I will try to rebase my patch on top of your work and report what happens.



================
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));
----------------
erichkeane wrote:
> Can you explain this more?  How does this work, and why don't we do that directly instead?
That's entangled with `calculateConstraintSatisfaction`. I actually tried to do it directly, but before passing expressions to this function `calculateConstraintSatisfaction` calls `IgnoreParenImpCasts()`, which strips away the lvalue-to-rvalue conversion.
And we need this conversion so that the evaluation that runs after this callback returns actually produces an r-value.

Note that the other call to `calculateConstraintSatisfaction` also calls `PerformContextuallyConvertToBool` after doing template substitution into the constraint expression.

I don't have full context on why it's the way it is, maybe there is a more fundamental change that helps with both cases.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2042
+        !SemaRef.CheckConstraintExpression(TransConstraint.get())) {
+      assert(Trap.hasErrorOccurred() && "CheckConstraintExpression failed, but "
+                                        "did not produce a SFINAE error");
----------------
erichkeane wrote:
> 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
> 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

Yeah, good point, I have update it.

I am not sure why would `CheckConstraintExpression` be called twice, could you elaborate? Note that we do not call `BuildNestedRequirement` anymore and use placement new directly to avoid extra template instantiations. Instead we call `CheckConstraintExpression` directly to account for any errors.


================
Comment at: clang/test/SemaTemplate/concepts-PR54629.cpp:10
+int main() {
+  A<int> a;
+}
----------------
erichkeane wrote:
> 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?
> 
> 
> 
I have added the test for primary template vs specialization.
Keeping a comment open to add a test for two specializations too, I will do this a bit later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127487