erichkeane added inline comments.
================
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));
----------------
ilya-biryukov wrote:
> 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.
Hmm... my understanding is we DO need these to be a boolean expression
eventually, since we have to test them as a bool, so that is why the other
attempts the converesion. If you think of any generalizations of this, it
would be appreciated, I'll think it through as well.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2042
+ !SemaRef.CheckConstraintExpression(TransConstraint.get())) {
+ assert(Trap.hasErrorOccurred() && "CheckConstraintExpression failed, but
"
+ "did not produce a SFINAE error");
----------------
ilya-biryukov wrote:
> 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.
This check does not seem to cause a 'return' to the function, but then falls
through to the version on 2052, right?
`CheckConstraintExpression`/`CheckConstraintSatisfaction`(i think?) ends up now
having to re-instantiate every time it is called, so any ability to cache
results ends up being beneficial here.
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
