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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
