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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
