[PATCH] D41910: [Concepts] Constrained partial specializations and function overloads.

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

rsmith added a comment.

(Didn't finish the review, but I have to run now.)



================
Comment at: include/clang/AST/DeclTemplate.h:742
   findSpecializationImpl(llvm::FoldingSetVector<EntryType> &Specs,
-                         ArrayRef<TemplateArgument> Args, void *&InsertPos);
+                         void *&InsertPos, ProfileArguments... ProfileArgs);
 
----------------
`ProfileArguments &&...ProfileArgs`?


================
Comment at: include/clang/AST/DeclTemplate.h:1989-2004
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    llvm::SmallVector<const Expr *, 3> AC;
+    getAssociatedConstraints(AC);
+    Profile(ID, getTemplateArgs().asArray(), AC, getASTContext());
+  }
+
+  static void
----------------
Hmm, is this the right set of things to be profiling? The relevant rule should be that the //template-head//s are equivalent and the //simple-template-id//s are equivalent, and checking the associated constraints isn't sufficient to check the former. Should we be profiling the template parameter list and template arguments instead?


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2469-2470
+def note_could_not_normalize_argument_substitution_failed : Note<
+  "when substituting into %0 %1. Make sure concept arguments are "
+  "valid for any substitution">;
 
----------------
Please don't join sentences with periods like this; it looks strange given that we generally render diagnostics in lowercase, and not as full sentences). I'm also not sure what the second sentence here is telling the user. (What do you mean by "any"? "some", or "all", or substitutions the program requires, or something else?)

We generally use "while" not "when" for these "here's what I was doing" notes.


================
Comment at: lib/Sema/SemaConcept.cpp:484-486
+  static llvm::Optional<NormalizedConstraint> fromConstraintExpr(
+      Sema &S, const Expr *E, TemplateDecl *TD = nullptr,
+      const ASTTemplateArgumentListInfo *ParameterMapping = nullptr) {
----------------
The specification says we substitute into the parameter mapping bottom-up (starting with atomic constraints and then substituting as they get used in enclosing constraints), but you're substituting top-down. That's not equivalent: specifically, the rules say that we should not perform substitution for parameters that are not used in the expansion of a concept ([temp.constr.atomic]p1: "[...] a mapping from the template parameters **that appear within E** to template arguments involving the [...]"). Example:

```
template<typename T> concept True = true;
template<typename T> concept Foo = True<T*>;
template<typename T> concept Bar = Foo<T&>;
template<typename T> void f() requires Bar<T>;
```

Here, the `true` atomic constraint has no parameter mappings, so we should never perform the substitution that would form `T&*`, and so shouldn't reject this for forming a pointer to a reference.

I think it's sufficient to first determine which parameters are used in a concept (perhaps when the concept is defined), and then form a parameter mapping top-down skipping the unused arguments, but you need to be careful to do that recursively (`Bar` doesn't use its `T` because `Foo` doesn't use its `T`).


================
Comment at: lib/Sema/SemaConcept.cpp:777-778
+
+static bool subsumes(Sema &S, ArrayRef<const Expr *> P,
+                     ArrayRef<const Expr *> Q) {
+  // C++ [temp.constr.order] p2
----------------
There are a bunch of ways we can optimize this, but let's get the straightforward approach landed first :)


================
Comment at: lib/Sema/SemaTemplate.cpp:3772
+        && (!Context.getLangOpts().ConceptsTS
+            || !TemplateParams->hasAssociatedConstraints())) {
       // C++ [temp.class.spec]p9b3:
----------------
`&&` and `||` on prior line, please.


Repository:
  rC Clang

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

https://reviews.llvm.org/D41910