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

[Shafik Yaghmour via Phabricator via cfe-commits]

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 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.
This is happening in `Sema::ActOnCallExpr` here:

```cpp
 // Diagnose uses of the C++20 "ADL-only template-id call" feature in earlier
  // language modes.
  if (auto *ULE = dyn_cast<UnresolvedLookupExpr>(Fn)) {
    if (ULE->hasExplicitTemplateArgs() &&
        ULE->decls_begin() == ULE->decls_end()) {
      Diag(Fn->getExprLoc(), getLangOpts().CPlusPlus20
                                 ? diag::warn_cxx17_compat_adl_only_template_id
                                 : diag::ext_adl_only_template_id)
          << ULE->getName();
    }
  }
```

if I check:

```console
expr CodeSynthesisContexts->rbegin()->Entity
```

That is indeed the `ClassTemplateDecl` and we do have `MultiExprArg ArgExprs` there as well.

The call right before this `TemplateInstantiator::RebuildCallExpr`


================
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}}
----------------
rsmith wrote:
> 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.)
They are not needed to trigger the issue, I removed them. I was torn about how faithful to remain to the original test case.


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

https://reviews.llvm.org/D148474