[PATCH] D44352: [Concepts] Type Constraints
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 8 17:19:45 PST 2020
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
More information about the cfe-commits
mailing list