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

Wed Aug 31 21:59:02 PDT 2022

ychen added a comment.

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052