ychen accepted this revision. ychen added a comment. This revision is now accepted and ready to land.
In D133052#3763983 <https://reviews.llvm.org/D133052#3763983>, @luken-google wrote: > 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. LGTM. Thanks. That's what I have in mind. The patch needs a rebase though. For future reference, I want to mention that the infinite recursion could happen in many other ways inside require expression. I'm not sure it is possible or worthwhile to check them all. I think we should do this check for the copy elision path because making conv-functions candidates are merely Clang's implementation detail about copy elision, the standard does not dictate *how* to perform copy elision. So avoiding infinite recursion in this path is better for user experiences. 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
