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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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 template arguments that we're using to instantiate, which seems like important information.

Do you know where we're creating the `InstantiatingTemplate` instance corresponding to this diagnostic? Usually we pass in the declaration whose definition we're instantiating for a `TemplateInstantiation` record; I wonder if we could do the same for this case.


================
Comment at: clang/test/SemaCXX/cxx1z-copy-omission.cpp:175
+
+// Make sure we don't consider conversion functions for guaranteed copy elision
+namespace GH39319 {
----------------
We should also test that the right function is actually being selected and called. For example:

```
struct A {
  A();
  A(const A&) = delete;
};
struct B {
  operator A();
} C;
A::A() : A(C) {}
```
... should be rejected because it calls the deleted copy constructor. (Or you could test this via constant evaluation or look at the generated code, but this is probably the simplest way.)


================
Comment at: clang/test/SemaCXX/cxx1z-copy-omission.cpp:193
+template<typename A3>
+class B3 : A3 // expected-error {{expected '{' after base class list}}
+  template<bool = C3<B3>()> // expected-warning 2{{use of function template name with no prior declaration in function call with explicit}}
----------------
Do you need to omit the `{` here as part of this test? This will cause problems for people editing the file due to mismatched braces, and makes this test fragile if our error recovery changes. It's best for the test case to be free of errors other than the one(s) being exercised. (Same for missing `;` errors below.)

If these errors are necessary to trigger the bug you found, I wonder if there's a problem with our error recovery. (Maybe we create a "bad" `InstantiatingTemplate` record during error recovery, for example.)


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

https://reviews.llvm.org/D148474