[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.

My only ask is to add a few more tests (see the relevant comment), otherwise LG.



================
Comment at: clang/lib/Sema/SemaOverload.cpp:1293
+    // templates first; the remaining checks follow.
+    bool SameTemplateParameterList = TemplateParameterListsAreEqual(
+        NewTemplate->getTemplateParameters(),
----------------
Hopefully this does not regress performance too much now that we keep matching template even if we don't need the result.
No need to take any action (the code looks simpler this way), just something to keep in mind in case it backfires.


================
Comment at: clang/test/SemaTemplate/concepts-using-decl.cpp:3
+
+namespace heads_without_concepts {
+struct base {
----------------
NIT: maybe move this to the end of the file?
I would expect concept-specific tests to go first given the name of the file.
It's still useful to capture the discovered corner-cases, obviously, just not as the first thing, maybe.


================
Comment at: clang/test/SemaTemplate/concepts-using-decl.cpp:112
+  expect<1>(baz{}.foo<Empty>()); // expected-error {{call to member function 'foo' is ambiguous}}
+}
+}
----------------
Could you also check that `requires` clauses and constraints in template parameters do not hide each other?
```
template <IsEmpty T> ...
// vs
template <class T> requires IsEmpty<T> ...
// vs
template <class T> void foo() requires IsEmpty<T> ...
```
Should not hide each other.

Another interesting case (that probably does not yet work):
```
struct base { template <class T> void foo(); };
struct derived : base { 
  using base::foo; 
  template <IsEmpty T> void foo();
};
```
^^ `derived().foo<Empty>()` will probably produce an ambiguity now (as we don't have an explicit requires clause here). I don't think it's worth fixing now, but keeping a test for it with a FIXME seems reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136440