[PATCH] D149264: GH62362: Handle constraints on "Using" inherited constructors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Confirmed this is all fixed by your patch.  Thanks!



================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:138-144
+                        bool SkipForSpecialization,
+                        bool ForConstraintInstantiation) {
   if (!ClassTemplSpec->isClassScopeExplicitSpecialization()) {
     // We're done when we hit an explicit specialization.
     if (ClassTemplSpec->getSpecializationKind() == TSK_ExplicitSpecialization &&
-        !isa<ClassTemplatePartialSpecializationDecl>(ClassTemplSpec))
+        !isa<ClassTemplatePartialSpecializationDecl>(ClassTemplSpec) &&
+        !ForConstraintInstantiation)
----------------
rsmith wrote:
> rsmith wrote:
> > It seems to me that a namespace-scope explicit specialization shouldn't pick up template arguments, even during constraint checking:
> > 
> > ```
> > template<typename T> struct A {};
> > template<> struct A<int> {
> >   template<typename U> void f() requires U::value;
> > };
> > ```
> > Constraint checking for `A<X>::f<Y>` should produce a single level of template arguments, `<Y>`, not two layers `<X>, <Y>`, because `U` in `U::value` is a depth-0 index-0 parameter.
> Complete testcase:
> ```
> struct X {}; struct Y { static constexpr bool value = true; };
> template<typename T> struct A {};
> template<> struct A<X> {                          
>   template<typename U> void f() requires U::value;
> };                                                                   
> void g(A<X> a) { a.f<Y>(); }
> ```
Ah! Thanks for the reproducer.  I was having a really hard time determining a case where this would break, everything I came up with was blocked by https://reviews.llvm.org/D146178

The difficulty was that the parent of the inherited constructor was an inline definition of the explicit specialization, so it DID have depth, but it wasn't clear how to differentiate the two. I think your patch as listed fixes all the issues I was trying to fix as you said, so I'll abandon this, though I'll include the tests as an NFC/RAC patch for completeness.



================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:217-219
+  if (Ctor->isImplicit() && Ctor->isInheritingConstructor() &&
+      Ctor->getInheritedConstructor().getConstructor())
+    Ctor = Ctor->getInheritedConstructor().getConstructor();
----------------
rsmith wrote:
> These checks can be simplified.
Ah, thanks!  I must have been looking at the wrong thing, I wasn't convinced that the `getInheritedConstructor` call was always legal, but looking again, I see that should have been obvious.  


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

https://reviews.llvm.org/D149264