[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2854-2858
-template <>
-bool DeducedArgsNeedReplacement<ClassTemplatePartialSpecializationDecl>(
-    ClassTemplatePartialSpecializationDecl *Spec) {
-  return !Spec->isClassScopeExplicitSpecialization();
-}
----------------
rsmith wrote:
> It's not obvious to me what this was doing, so I'm not sure whether it's correct to remove it. Can you explain? It makes me uncomfortable that we would treat class template partial specializations and variable template partial specializations differently, at least without a good explanation for why that's appropriate -- perhaps we should apply this same set of changes to variable template partial specializations too, and remove this mechanism entirely?
Absolutely!  And I suspect at a certain point we are going to want to remove this entirely, it is a bit of a hack (and I couldn't come up with a good repro that this caused a problem, and the `VarTemplateSpecializationDecl` code does a lot of work).

  
Basically, when doing the `CheckDeducedArgumentConstraints` below, we get the instantiation args list (see this being used on 2869).  

However, the 'inner most' args when this is a Partial Specialization end up being 'wrong', since they come from the partial specialization.  This is used to determine whether we need to replace them with the args we already have. However, this patch now makes it so `ClassTemplatePartialSpecializationDecl`s no longer generate template arguments in `getTemplateInstantiationArgs` (which has seen a bit of a refactor since you were here last, since it was a bit of a difficult to understand function that didn't seem to cover many cases well, and had a lot of repetation).




================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:322
+      // they mean.
+      R = Response::UseNextDecl(PartialClassTemplSpec);
     } else if (const auto *ClassTemplSpec =
----------------
rsmith wrote:
> Can we produce a "done" response instead here? It doesn't seem right to carry on to the next level here without adding a level of template arguments -- if any further arguments were somehow collected, they would be collected at  the wrong template depth. But, as far as I can determine, we should never encounter a partial specialization declaration except in cases where there is nothing remaining to be substituted (all outer substitutions would be identity substitutions). I think the same thing applies when we reach a class template declaration (the first branch in `HandleRecordDecl`) -- we should just be able to stop in that case too, rather than collecting some identity template arguments and carrying on to an outer level.
> 
> If we can't produce a "done" response here for some reason, then we should collect a level of identity template arguments instead.
For constraints, the arguments are relative to namespace scope as far as I can tell.  If we ended up 'stopping' we would miss references to the outer template, so something like:

``` 
template<typename T>
struct Outer {

  template<typename U>
  struct Inner {
     void foo() requires (is_same<T, U>){}
  };

};

```

Right?  The substitution fo 'T' would be invalid at that point?

Also, what do you mean by 'identity' template arguments?  I'm not sure I follow.


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

https://reviews.llvm.org/D147722