rsmith added inline comments.
================ Comment at: lib/AST/DeclTemplate.cpp:203 + +Expr *TemplateDecl::getAssociatedConstraints() { + return getOrCollectAssociatedConstraints(getASTContext(), ---------------- Rather than producing an `Expr*` (which will presumably eventually need to contain synthetic `&&` expressions), this should produce a vector of constraints (notionally and'd together), which will (eventually) be either constraint-expressions or some representation of "the constraints implied by this template parameter". If you do that, I don't think there's any reason to cache this information on the `TemplateDecl`; it should be cheap enough to recompute when we need it. ================ Comment at: lib/Sema/SemaConcept.cpp:47-58 + const Expr *NewAC) { + if (!(NewAC || OldAC)) + return true; // Nothing to check; no mismatch. + if (NewAC && OldAC) { + llvm::FoldingSetNodeID OldACInfo, NewACInfo; + NewAC->Profile(NewACInfo, Context, /*Canonical=*/true); + OldAC->Profile(OldACInfo, Context, /*Canonical=*/true); ---------------- I'm sure we already have this "profile two expressions and check they are canonically equivalent" functionality factored out //somewhere//. Is there nothing like that that's appropriate to reuse here? ================ Comment at: lib/Sema/SemaTemplate.cpp:2145-2154 if (RemoveDefaultArguments) { - for (TemplateParameterList::iterator NewParam = NewParams->begin(), - NewParamEnd = NewParams->end(); - NewParam != NewParamEnd; ++NewParam) { - if (TemplateTypeParmDecl *TTP = dyn_cast<TemplateTypeParmDecl>(*NewParam)) + for (NamedDecl *NewParam : *NewParams) { + if (TemplateTypeParmDecl *TTP = dyn_cast<TemplateTypeParmDecl>(NewParam)) TTP->removeDefaultArgument(); else if (NonTypeTemplateParmDecl *NTTP - = dyn_cast<NonTypeTemplateParmDecl>(*NewParam)) + = dyn_cast<NonTypeTemplateParmDecl>(NewParam)) NTTP->removeDefaultArgument(); ---------------- Unrelated cleanup: please commit this separately. ================ Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:3141-3148 + Expr *InstRequiresClause = nullptr; + if (Expr *E = L->getRequiresClause()) { + ExprResult Res = SemaRef.SubstExpr(E, TemplateArgs); + if (Res.isInvalid() || !Res.isUsable()) { + return nullptr; + } + InstRequiresClause = Res.get(); ---------------- This is wrong (we should never substitute into a constraint-expression except as part of satisfaction or subsumption checks). Please at least add a FIXME. ================ Comment at: lib/Serialization/ASTReaderDecl.cpp:2003-2006 + bool HasAssociatedConstraints = Record.readInt(); + if (HasAssociatedConstraints) { + AssociatedConstraints = Record.readExpr(); + } ---------------- There doesn't seem to be a good reason to serialize the cached associated constraints, even if we want to keep them (which I don't think we do). ================ Comment at: lib/Serialization/ASTWriter.cpp:5856 for (const auto &P : *TemplateParams) - AddDeclRef(P); + AddDeclRef(P); // TODO: Concepts - constrained parameters. + if (const Expr *RequiresClause = TemplateParams->getRequiresClause()) { ---------------- What would be left "to do" here? For constrained parameters, there may be something interesting to track on the `*Template*ParmDecl`, but not here, I wouldn't think, Repository: rC Clang https://reviews.llvm.org/D41284 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits