[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

Thu Sep 1 07:15:32 PDT 2022

luken-google added a comment.

In D133052#3763201 <https://reviews.llvm.org/D133052#3763201>, @ychen wrote:

> In D133052#3763041 <https://reviews.llvm.org/D133052#3763041>, @luken-google wrote:
>
>> In D133052#3762596 <https://reviews.llvm.org/D133052#3762596>, @ychen wrote:
>>
>>> Like mentioned in https://stackoverflow.com/questions/68853472/is-this-a-bug-in-clangs-c20-concepts-implementation-unnecessary-checking-of, could we not go down the path of considering conversion candidates? It seems that's blessed by the standard.
>>
>> If I'm understanding the code correctly, the intent of this patch is to definitely consider conversion candidates, only to exclude those conversion candidates that are templated methods where the From type is the same as the To type, which to me mean they are possibly redundant?
>
> Excluding them is basically saying "because it may be a redundant conversion, we should not consider it as the best via function." which doesn't seem correct to me.
>
> I think the straightforward approach would be to check if we're in the `ConstraintCheck` instantiation context, and if so check if any template parameter is constrained by the same concept. However, I'm worried about the overhead. So I'd prefer to skip this add-conv-candicates-for-copy-elision path (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaInit.cpp#L4012-L4053) during the concept check. The compiler guarantees copy elision under certain conditions (C++17) but I could not think of any situation that users want to or could check copy elision inside the concept. So I think we're safe.

Thanks for your suggestion, I didn't know about the context member in Sema. I agree I think this is a much better approach than my original. While looping the code is in the `RequirementInstantiation` context, so that's the one I've keyed off here. Please let me know if this is what you had in mind.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052