[PATCH] D136440: [clang] Do not hide base member using-decls with different template head.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for the change. All the behavior changes look reasonable to me, but I'm struggling to understand whether we follow the standard closely here.
Left a comment to discuss this in depth.



================
Comment at: clang/lib/Sema/SemaOverload.cpp:1238
 bool Sema::IsOverload(FunctionDecl *New, FunctionDecl *Old,
-                      bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs,
+                      bool IsForUsingDecl, bool ConsiderCudaAttrs,
                       bool ConsiderRequiresClauses) {
----------------
`UseMemberUsingDeclRules` is used everywhere in the code and in the comments for calls to `IsOverload `.

Let's maybe change the name in the header instead to avoid chasing all call sites?


================
Comment at: clang/lib/Sema/SemaOverload.cpp:1298
+                                              New->getDeclaredReturnType());
+    auto IsConstructorOrAssignment = [](FunctionDecl *FD) {
+      auto *CMD = dyn_cast<CXXMethodDecl>(FD);
----------------
Lambda is only used once. Could you inline it to make the code simpler?
```
bool IsConstructorOrAssignment = false;
if (auto *CMD = dyn_cast<CXXMethodDecl>(FD); CMD && (...)) {
  IsConsutrctorOrAssignment = true;
}
```


================
Comment at: clang/lib/Sema/SemaOverload.cpp:1307
+    // C++ [namespace.udecl]p4:
+    //   The member from the base class is hidden or overridden by the
+    //   implicitly-declared copy/move constructor or assignment operator of the
----------------
How is this related to checking whether the template parameter lists check?
I seems to be missing the logical connection that goes from the standard wording to the code. Could you explain?

An example that I have in mind is:
```
struct A {
    template <class T, class U = int> A(T);
    template <class T, class U = int> int foo(T);
};
struct B : A {
    using A::A;
    template <class T> B(T);

    using A::foo;
    template <class T> int foo(T);
};

namespace detail {
    template <class T> int bar(T);
}
using detail::bar;
template <class T, class U = int> int bar(T);

int a = bar(10); // this is definitely ambiguous.

B b(10); // should this be ambiguous?
int c = b.foo(10); // should this be ambiguous?
// Should constructors and functions behave differently? Why?
```

I [see](https://gcc.godbolt.org/z/z4Ko1ehPe) that both Clang and GCC treat member declarations differently, but I don't see why they should given that they are supposed to use the same "corresponds" terminology from the standard. What am I missing?


================
Comment at: clang/test/SemaTemplate/concepts-using-decl.cpp:110
+  expect<1>(bar2{}.foo<Empty>());
+  // FIXME: Candidates from using-decls should be dropped in case of ambiguity.
+  expect<1>(baz{}.foo<Empty>()); // expected-error {{call to member function 'foo' is ambiguous}}
----------------
Why should they be dropped? Could you provide pointers from the standard?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136440