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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
