[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

aaron.ballman accepted this revision. aaron.ballman added a comment. The changes look reasonable to me (@rsmith had a lot of comments, but I think you addressed them; it would be nice if he could confirm), but should definitely come with a release note. So LGTM modulo those nits and any

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

dim accepted this revision. dim added a comment. This revision is now accepted and ready to land. Yes, please. :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/ https://reviews.llvm.org/D148474 ___ cfe-commits mailing list

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added a comment. ping @dim @rsmith @aaron.ballman CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/ https://reviews.llvm.org/D148474 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added a comment. ping @dim @rsmith @aaron.ballman CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/ https://reviews.llvm.org/D148474 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/ https://reviews.llvm.org/D148474 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/ https://reviews.llvm.org/D148474 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik updated this revision to Diff 557569. shafik added a comment. - Integrate fix from https://github.com/llvm/llvm-project/pull/67373 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/ https://reviews.llvm.org/D148474 Files: clang/lib/Sema/SemaInit.cpp

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added a comment. @dim the crash should be fixed by landing: https://github.com/llvm/llvm-project/pull/67373 I need to rebase and test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/ https://reviews.llvm.org/D148474 ___

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added a comment. In D148474#4490041 , @dim wrote: > FWIW, this fix works for the test cases I had via > https://bugs.freebsd.org/269067 and > https://github.com/llvm/llvm-project/issues/60182, but I got the following > failure during check-all:

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

dim added a comment. In D148474#4490041 , @dim wrote: > FWIW, this fix works for the test cases I had via > https://bugs.freebsd.org/269067 and > https://github.com/llvm/llvm-project/issues/60182, but I got the following > failure This was with

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

dim requested changes to this revision. dim added a comment. This revision now requires changes to proceed. FWIW, this fix works for the test cases I had via https://bugs.freebsd.org/269067 and https://github.com/llvm/llvm-project/issues/60182, but I got the following failure during check-all:

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/ https://reviews.llvm.org/D148474 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/ https://reviews.llvm.org/D148474 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/ https://reviews.llvm.org/D148474 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:746 + diag::note_template_class_instantiation_here) +<< CTD << Active->InstantiationRange; } else { rsmith wrote: > This diagnostic won't

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik updated this revision to Diff 514091. shafik marked 3 inline comments as done. shafik added a comment. - Fix up A3 test to be less malformed - Fix other test so we confirm we are attempting to call the right contructor CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

rsmith added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:746 + diag::note_template_class_instantiation_here) +<< CTD << Active->InstantiationRange; } else { This diagnostic won't include the

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:743 << FD << Active->InstantiationRange; + } else if (ClassTemplateDecl *CTD = dyn_cast(D)) { +Diags.Report(Active->PointOfInstantiation, I added this

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik created this revision. shafik added reviewers: rsmith, aaron.ballman, erichkeane, dim. Herald added a project: All. shafik requested review of this revision. `ResolveConstructorOverload` needs to check properly if we are going to use copy elision we can't use a conversion function. This