rsmith added inline comments.
================ Comment at: include/clang/AST/ASTNodeTraverser.h:518 + if (const auto *TC = D->getTypeConstraint()) + if (TC->wereArgumentsSpecified()) + for (const auto &ArgLoc : TC->getTemplateArgsAsWritten()->arguments()) ---------------- For consistency with `DeclRefExpr`, I think this function should be called `hasExplicitTemplateArgs`. ================ Comment at: include/clang/AST/RecursiveASTVisitor.h:1781 TRY_TO(TraverseType(QualType(D->getTypeForDecl(), 0))); + if (const auto *TC = D->getTypeConstraint()) + if (TC->wereArgumentsSpecified()) ---------------- We should visit the nested name specifier and concept name too. Perhaps it'd make sense to add a `TraverseConceptReference` extension point and call it from here and from visitation of a `ConceptSpecializationExpr`? ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2508 +def err_template_template_parameter_not_at_least_as_constrained : Error< + "template template argument %0 must not be more constrained than template " + "template parameter %1">; ---------------- "must not be" -> "is" ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2512 +def err_type_constraint_non_type_concept : Error< + "concept named in type constraint must be a type concept">; +def err_type_constraint_missing_arguments : Error< ---------------- "must be" -> "is not" ================ Comment at: lib/AST/ASTContext.cpp:703-707 + Expr *NewIDC = ConceptSpecializationExpr::Create(*this, + CSE->getNestedNameSpecifierLoc(), CSE->getTemplateKWLoc(), + CSE->getConceptNameInfo(), CSE->getFoundDecl(), + CSE->getNamedConcept(), CSE->getTemplateArgsAsWritten(), + NewConverted, ConstraintSatisfaction()); ---------------- Canonicalization should presumably also remove / canonicalize the "as-written" parts here: no `NestedNameSpecifierLoc`, no `template` keyword location, the found decl should just be the named concept itself, and the explicit template arguments should be canonicalized. (We don't want the locations and "as-written" parts from the first instance of a particular template template parameter that we happen to encounter to be reachable through later declarations of identical template template parameters.) ================ Comment at: lib/AST/ASTContext.cpp:709-716 + if (auto *OrigFold = dyn_cast<CXXFoldExpr>(IDC)) + NewIDC = new (*this) CXXFoldExpr(OrigFold->getType(), + OrigFold->getBeginLoc(), NewIDC, + BinaryOperatorKind::BO_LAnd, + OrigFold->getEllipsisLoc(), + OrigFold->getRHS(), + OrigFold->getEndLoc(), ---------------- Similarly, we presumably shouldn't be preserving locations here through canonicalization. ================ Comment at: lib/AST/DeclPrinter.cpp:1048-1049 Out << "typename"; + else if (const TypeConstraint *TC = TTP->getTypeConstraint()) + TC->print(Out, Policy); else ---------------- Nit: I think this would read more naturally if the type constraint case were first. ================ Comment at: lib/AST/TextNodeDumper.cpp:1680-1682 if (D->wasDeclaredWithTypename()) OS << " typename"; + else if (const auto *TC = D->getTypeConstraint()) { ---------------- As above, I think this would be clearer if the "type constraint" and "typename" cases were reversed. ================ Comment at: lib/AST/TextNodeDumper.cpp:1689 + OS << ")"; + } + } else ---------------- It'd be useful to dump either the explicit arguments or the immediately-declared constraint here. ================ Comment at: lib/Parse/ParseExprCXX.cpp:162 + assert(!LastII && "want last identifier but have already annotated scope"); assert(!LastII && "want last identifier but have already annotated scope"); assert(!MayBePseudoDestructor && "unexpected annot_cxxscope"); ---------------- We don't need both of these :) ================ Comment at: lib/Parse/ParseExprCXX.cpp:487-490 + if (OnlyNamespace) + // We can't have template-ids as part of a namespace scope specifier, + // the scope specifier must end here. + break; ---------------- Is this necessary for this change in particular? (I'd expect `isTemplateName` to be able to cope with this corner case.) ================ Comment at: lib/Parse/ParseTemplate.cpp:552 + if (Tok.is(tok::annot_template_id) && + static_cast<TemplateIdAnnotation *>(Tok.getAnnotationValue())->Kind == ---------------- Have we necessarily annotated a //template-id// at this point, if necessary? Usually functions that want to look at template-id annotations perform that annotation themselves rather than imposing a side-condition on their caller to do it before calling them. ================ Comment at: lib/Parse/ParseTemplate.cpp:619-644 + // We could be facing a type-constraint, which (could) start a type parameter. + // Annotate it now (we might end up not using it if we determine this + // type-constraint is in fact part of a placeholder-type-specifier of a + // non-type template parameter. - Diag(Tok.getLocation(), diag::note_meant_to_use_typename) - << FixItHint::CreateReplacement(CharSourceRange::getCharRange( - Tok.getLocation(), Tok.getEndLoc()), - "typename"); + bool WasScopeAnnotation = Tok.is(tok::annot_cxxscope); + CXXScopeSpec SS; ---------------- As mentioned above, I'd expect this all to be done by `isStartOfTemplateTypeParameter` itself. ================ Comment at: lib/Parse/ParseTemplate.cpp:679-681 +/// optional scope specifier. A type-constraint names a concept along with an +/// optional set of template arguments, and denotes a placeholder that accepts +/// types that (along with the set of arguments, if any) satisfy the concept. ---------------- I'd drop the second sentence of this comment. We don't need to talk about how the language works in the parser, just how to parse it. ================ Comment at: lib/Parse/ParseTemplate.cpp:683-686 +/// type-constraint: +/// nested-name-specifier[opt] concept-name ... identifier[opt] +/// nested-name-specifier[opt] concept-name identifier[opt] +/// default-template-argument[opt] ---------------- This is out-of-date compared to the C++20 wording. ================ Comment at: lib/Parse/ParseTemplate.cpp:801-811 + NamedDecl *NewDecl = Actions.ActOnTypeParameter(getCurScope(), + TypenameKeyword, EllipsisLoc, + KeyLoc, ParamName, NameLoc, + Depth, Position, EqualLoc, + DefaultArg, + TypeConstraint != nullptr); + ---------------- Does this need to be done as two separate steps? (I wonder if combining them might allow us to remove the complexity of having a "has an invalid type constraint" state from `TemplateTypeParmDecl`.) ================ Comment at: lib/Parse/ParseTemplate.cpp:1286 TemplateArgList TemplateArgs; - bool Invalid = ParseTemplateIdAfterTemplateName(false, LAngleLoc, - TemplateArgs, - RAngleLoc); - - if (Invalid) { - // If we failed to parse the template ID but skipped ahead to a >, we're not - // going to be able to form a token annotation. Eat the '>' if present. - TryConsumeToken(tok::greater); - // FIXME: Annotate the token stream so we don't produce the same errors - // again if we're doing this annotation as part of a tentative parse. - return true; + if (!TypeConstraint || Tok.is(tok::less)) { + bool Invalid = ParseTemplateIdAfterTemplateName(false, LAngleLoc, ---------------- Interesting. So we're using a `tok::annot_template_id` annotation for this even in the case where we have no template arguments, so it's not actually a //template-id//? That's a bit weird, but I'm not necessarily opposed if it keeps things simpler. Please at least mention that in the comment on `ANNOTATION(template_id)` in Basic/TokenKinds.def, and also in the doc comment for class `TemplateIdAnnotation`. ================ Comment at: lib/Sema/SemaCXXScopeSpec.cpp:620 + if (Found.empty() && !ErrorRecoveryLookup && !SuppressDiagnostics + && !getLangOpts().MSVCCompat) { // We haven't found anything, and we're not recovering from a ---------------- Nit: `&&` on the previous line, please. ================ Comment at: lib/Sema/SemaConcept.cpp:847-849 +bool Sema::IsAtLeastAsConstrained(NamedDecl *D1, ArrayRef<const Expr *> AC1, + NamedDecl *D2, ArrayRef<const Expr *> AC2, + bool NoCache) { ---------------- Please go ahead and commit this renaming (as a separate change). ================ Comment at: lib/Sema/SemaTemplate.cpp:1059 + FixItHint::CreateInsertion(ConstrainedParameter->getLocation(), + "<...>"); + return true; ---------------- Fixit hints on an error or warning should be valid C++ code that fixes the problem, and you should recover as if the fix were applied. See http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints for the rules and rationale. We don't have a way of annotating text on a diagnostic that isn't a fixit hint (although we probably should); for now, let's just omit the hint. (Maybe add a FIXME though?) ================ Comment at: lib/Sema/SemaTemplate.cpp:1087 + SourceLocation EllipsisLoc) { + // C++2a [temp]p6: + // [...] If Q is of the form C<A1, ⋯, An>, then let E′ be C<T, A1, ⋯, An>. ---------------- This (and the later references to it) have the wrong section and paragraph listed: this is [temp.param]p4 not [temp]p6. ================ Comment at: lib/Sema/SemaTemplate.cpp:1089 + // [...] If Q is of the form C<A1, ⋯, An>, then let E′ be C<T, A1, ⋯, An>. + // Otherwise, let E′ be C<T>. [...] + const ASTTemplateArgumentListInfo *ArgsAsWritten = ---------------- Please replace these characters (ellipsis and prime) with ASCII; bad things may happen to these characters when this file is viewed from an editor that defaults to something other than UTF-8 (which is quite likely on Windows). ================ Comment at: lib/Sema/SemaTemplate.cpp:1123 + // C++2a [temp]p6: + // [...] If T is not a pack, then E is E′, otherwise E is (E′ && ...). + // ---------------- Similarly here. ================ Comment at: lib/Sema/SemaTemplate.cpp:1967-1983 + if (const auto *TC = TTP->getTypeConstraint()) { + TemplateArgumentListInfo TransformedArgs; + const auto *ArgsAsWritten = TC->getTemplateArgsAsWritten(); + if (SemaRef.Subst(ArgsAsWritten->getTemplateArgs(), + ArgsAsWritten->NumTemplateArgs, TransformedArgs, + Args)) { + ExprResult InstantiatedConstraint = ---------------- This will substitute into each of the template arguments twice. It would be better to rebuild the immediately-declared constraint from scratch (and that might even be necessary for correctness in some cases: if a template argument contains a //lambda-expression//, we should introduce only one closure type, not two -- and will in any case avoid duplicate warnings). ================ Comment at: lib/Sema/SemaTemplate.cpp:7047 // is at least as specialized as the template-argument A. - if (getLangOpts().RelaxedTemplateTemplateArgs) { + if (getLangOpts().RelaxedTemplateTemplateArgs || getLangOpts().ConceptsTS) { // Quick check for the common case: ---------------- We shouldn't gate this behind `ConceptsTS`. Let's just start turning `RelaxedTemplateTemplateArgs` on unconditionally. (This breaks at least one testcase, but it's not clear that it's a real world problem.) As a separate change, though :) ================ Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:2387-2388 + } + ExprResult Result = + SemaRef.SubstExpr(TC->getImmediatelyDeclaredConstraint(), TemplateArgs); + if (Result.isInvalid()) ---------------- As above, this should be recreated from scratch, not substituted. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D44352/new/ https://reviews.llvm.org/D44352 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits