[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:775-776
+static bool IsInsideImplicitFullTemplateSpecialization(const DeclContext *DC) {
+  auto *CTSDecl = dyn_cast_or_null<ClassTemplateSpecializationDecl>(
+      DC->getOuterLexicalRecordContext());
+  return CTSDecl && !isa<ClassTemplatePartialSpecializationDecl>(CTSDecl) &&
----------------
This doesn't look right to me; there could be a class template nested inside a non-templated class, so I think you would need to walk up the enclosing `DeclContext`s one by one checking each in turn. But, we might be inside a function template specialization or variable template specialization instead, in some weird cases:

```
template<typename T> concept C = true;
template<typename T> auto L = []<C<T> U>() {};
struct Q {
  template<C<int> U> friend constexpr auto decltype(L<int>)::operator()() const;
};
```

... so I think we want a different approach than looking for an enclosing class template specialization declaration.

Can we skip this check entirely, and instead always compute and substitute the template instantiation arguments as is done below? That computation will walk the enclosing contexts for us in a careful way that properly handles cases like this lambda-in-variable-template situation. If we find we get zero levels of template argument list, we can skip doing the actual substitution as an optimization.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:1303
+        OldTemplate->getTemplateParameters(), false, TPL_TemplateMatch,
+        SourceLocation(), false /* PartialOrdering */);
     bool SameReturnType = Context.hasSameType(Old->getDeclaredReturnType(),
----------------
rsmith wrote:
> shafik wrote:
> > nit
> Just remove the final parameter; it has a default argument of `false` and no other call site passes `false` here.  (I'm working on removing this parameter in a different change.)
You can remove the `SourceLocation()` argument too; there's an identical default argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178