ilya-biryukov added a comment.
I've checked that the change works fine on top of D126907
<https://reviews.llvm.org/D126907>. The bug is still there after D126907
<https://reviews.llvm.org/D126907> and gets fixed by this.
Also, the merge conflict is actually minimal, no code changes intersect.
================
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));
----------------
royjacobson wrote:
> erichkeane wrote:
> > 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.
> Note we already have a related bug about this
> https://github.com/llvm/llvm-project/issues/54524
Yeah, they have to be bool and we actually check for that in
`CheckConstraintExpression`. The standard explicitly mentions only the
lvalue-to-rvalue conversion should be performed.
```
[temp.constr.atomic]p3 If substitution results in an invalid type or
expression, the constraint is
not satisfied. Otherwise, the lvalue-to-rvalue conversion (7.3.1) is performed
if necessary, and E shall be a constant expression of type bool.
```
However, in the calls to `calculateConstraintSatisfaction` we do a more generic
boolean conversion, but the comment in the other call site suggests this
probably accidental and we actually want a less generic conversion:
```
// Substitution might have stripped off a contextual conversion to
// bool if this is the operand of an '&&' or '||'. For example, we
// might lose an lvalue-to-rvalue conversion here. If so, put it back
// before we try to evaluate.
if (!SubstitutedExpression.isInvalid())
SubstitutedExpression =
S.PerformContextuallyConvertToBool(SubstitutedExpression.get());
```
I am happy to take a look at fixing the mentioned bug.
================
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:
> 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.
The number of calls to these functions is actually the same.
`CheckConstraintExpression` used to be called during
`CheckConstraintSatisfaction` (that does instantiations) for every atomic
constraint after the substitution. It merely checks that each constraint have a
bool type and does not do any substitutions, so it's pretty cheap anyway.
`CheckConstraintSatisfaction` was called inside `BuildNestedRequirement`, we
now call a different overload here directly that does not do any extra template
instantiations directly.
That way we end up doing the same checks without running recursive template
instantiations.
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
