[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:496
+llvm::Optional<MultiLevelTemplateArgumentList>
+Sema::SetupConstraintCheckingTemplateArgumentsAndScope(
+    FunctionDecl *FD, llvm::Optional<ArrayRef<TemplateArgument>> TemplateArgs,
----------------
erichkeane wrote:
> Ugh... it looks like all of this might just be 'wrong', and I have no idea how to fix it.  @rsmith  ANY advise you could give here would be unbelievably appreciated.
> 
> Basically, anything involving variables besides ParmVarDecls seems to be broken in some way:
> 
> 
> 
> ```
> template<typename T>
> void foo(T x) {
> 
> // This asserts because 'y' is not in the Scope's "FindInstantiationOf"
> [y = x]() requires(constraint<decltype(y)>){}
> 
> 
> // This asserts because 'Local' is not in the Scope's "FindInstantiationOf"
> T Local;
> []() requires(constraint<decltype(Local)>){}
> 
> // This gives a "stack nearly exhausted" error, followed by a bunch of "while checking constraint satisfaction for function 'foo' required here'", THEN crashes checking Constraints due to MLTAL being empty at one point (or perhaps just corrupt?). BUT the stack is only 40 deep at the crash.
> struct S {
> int local;
> void foo() requires(constraint<decltype(local)>){}
> }
> ```
> 
> FURTHER, this is also broken by this patch:
> ```
> template<typename T>
> struct S {
> T t;
> void foo() requires(constraint<decltype(t)>){}
> };
> void use() {
> S<int> s;
> s.foo();
> ```
> With "constraints not satisfied" "because substituted constraint expression is ill-formed: 'S::t' is not a member of 'class S<int>'"
> 
> Curiously, it works if 'foo' is itself a template.
> 
> 
> 
> 
> I can't help but think that this attempt to re-generate the instantiation is just BROKEN, and I have no idea how to fix it, or what the right approach is.  BUT I cannot help but think that I'm doing the 'wrong thing'.
> 
Note that each of those lambdas ALSO needs to be called there, I forgot the last ().


================
Comment at: clang/lib/Sema/TreeTransform.h:13002
+  ExprResult NewTrailingRequiresClause =
+      E->getCallOperator()->getTrailingRequiresClause();
 
----------------
cor3ntin wrote:
> That doesn't look right.
> At best if you don't transform the trailing return type you wouldn't refer to the transformed captures and that is the issue you are seeing with
> 
> ```
> // This asserts because 'y' is not in the Scope's "FindInstantiationOf"
> [y = x]() requires(constraint<decltype(y)>){}
> ```
> 
> But i suspect this should explode even more spectacularly in some cases? 
> If i understand correctly, it shouldn't be checked - but it still be substituted.... or am i missing something?
> 
>At best if you don't transform the trailing return type you wouldn't refer to the transformed captures and that is the issue you are seeing with

So the point of the patch here is that we cannot transform this until we need to 'check' this constraint.  I also missed the 'call' of the lambdas in each of those cases, so it does need to be 'checked', which means it needs to be 'substituted'.

>But i suspect this should explode even more spectacularly in some cases? 
>If i understand correctly, it shouldn't be checked - but it still be substituted.... or am i missing something?

I think it cannot be substituted either, because the type might be different by the time you get to the need to check, see here: https://godbolt.org/z/GxP9T6fa1


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

https://reviews.llvm.org/D119544