[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126907#3588708 <https://reviews.llvm.org/D126907#3588708>, @erichkeane wrote:

> In D126907#3588417 <https://reviews.llvm.org/D126907#3588417>, @ChuanqiXu wrote:
>
>> From what I can see, the crash reason would be the mismatch depth and the setting of MultiLevelTemplateArgumentList. In `::CheckConstraintSatisfaction`, the depth of `RawCompletionToken ` is 0, while the depth of corresponding MultiLevelTemplateArgumentList is 2. So the compiler would get the outermost template argument incorrectly (which is a template pack in the example) in TransformTemplateTypeParmType.
>>
>> The first though was that we should set the depth of `RawCompletionToken ` to 1 correctly. But I felt this might not be good later since in the normal process of template instantiation (without concepts and constraints), the depth of `RawCompletionToken ` is 0 indeed. What is different that time is the depth of corresponding `MultiLevelTemplateArgumentList ` is 1. So it looks like the process is constructed on the fly. It makes sense for the perspective of compilation speed.
>>
>> So I feel like what we should do here is in `Sema::CheckInstantiatedFunctionTemplateConstraints`, when we are computing the desired MultiLevelTemplateArgumentList, we should compute a result with depth of 1 in the particular case.
>>
>> ---
>>
>> Another idea is to not do instantiation when we're checking constraints. But I need to think more about this.
>
> I don't know of the 'another idea'?  We have to do instantiation before checking, and if we do it too early, we aren't doing the deferred instantiation.  And the problem is that the RawCompletionToken uses a concept to refer to 'itself'.  However, this version of 'itself' isn't valid, since the 'depth' is wrong once it tries to instantiate relative to the 'top level'.  However, this IS happening during instantiation?
>
> My latest thought is that perhaps the "IsEvaluatingAConstraint" should NOT happen at the Sema level (but at the instantiator level), since it ends up catching things it shouldn't?  I tried it and have a handful of failures of trying to check uninstantiated constraints, but I've not dug completely into it.

Yeah, we have to do instantiation before checking. My point is that it looks like we're doing **another** instantiation when we check the concepts. And my point is that if it we should avoid the second instantiation.


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

https://reviews.llvm.org/D126907