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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
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:
> > ilya-biryukov wrote:
> > > 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.
> > Hmm... I guess what I'm saying is: I would like it if we could minimize/reduce the calls to calcuateConstraintSatisfaction (in SemaConcept.cpp) that does the call to SubstConstraintExpr (or substExpr).  
> > 
> > That ends up being more expensive thanks to my other patch.
> Ah, yes, that makes sense. Note that `CheckConstraintSatisfaction` call on 2052 does not do any substitutions, it merely evaluates the expressions if they are not dependent.
> 
> I will have to look more closely into your patch to get a sense of why substituting to the constraint expressions is more expensive after it.
> 
> BTW, I was wondering if there is any reason to substitute empty template arguments to evaluate non-dependent constraints? (This is what `CalculateConstraintSatisfaction` does)
> I feels like merely evaluating those should be enough.
ah, I see!  Constraint expressions get 'more expensive' because now we actually have to do substitution EVERY time we evaluate, rather than just having an already non-dependent expression stored.  This is because the deferred constraint instantiation requires that we re-evaluate it just about every time we check a constraint.

Which version of `CalculateConstraintSatisfaction` are you referring to?  it DOES actually appear that the one you modify above doesn't actually DO any substitution.  it DOES looklike it does some checking to make sure we specifically are a boolean expr ([temp.constr.atompic]p1 ~ line 210), but it isn't clear to me what the logic there is either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127487