erichkeane added a comment. In D126907#3591195 <https://reviews.llvm.org/D126907#3591195>, @ChuanqiXu wrote:
> 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. Hmm... yeah, I think we actually want to skip the FIRST instantiation? I think the work I did above in Sema for `IsEvaluatingAConstraint` was too greedy, it ends up instantiating constraints on the 'while we are there' type things, rather than the things being currently checked. I found that if I can make it a state of the instantiators themselves, it seems to work, at least for your example. I've got it down to 1 'lit' test failure, but have been looking at the other problem first (below). The Github-Issue crash IS a regression from this patch, and I've minimized it sufficiently. I believe I have a hold on how to fix it, I just have to do so :) If I make any further progress, I'll clean this up and put it up here, even if it DOES have the lit test failure. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126907/new/ https://reviews.llvm.org/D126907 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
