Added support for type-constraints in template type parameters. Depends on D43357.
Details
Diff Detail
- Repository
- rC Clang
- Build Status
Buildable 33401 Build 33400: arc lint + arc unit
Event Timeline
Split TryParseConstrainedParameter and ParseConstrainedTemplateParameter in preparation for requries expressions.
lib/AST/ASTImporter.cpp | ||
---|---|---|
3708 ↗ | (On Diff #159378) | Please use the import function instead of VisitExpr. Calling VisitExpr would return immediately with an error, that's the default case of the StmtVisitor. ExpectedExpr CEOrErr = import(D->getConstraintExpression()); if (!CEOrErr) return CEOrErr.takeError(); R->setConstraintExpression(*CEOrErr); And could you please adjust the changes below too? |
lib/AST/ASTImporter.cpp | ||
---|---|---|
3708 ↗ | (On Diff #159378) | The code above works without the if (Expr *CE = D->getConstraintExpression()) too. The import returns nullptr for a nullptr (*CEOrErr will be nullptr and no error). |
- Fixed bad ordering of associated constraints
- Fixed ignoring of ScopeSpecifier in constrained parameters
- Adjustments for template template parameter handling
- Fixed constrained template template parameter and argument handling
This needs revision to reflect changes between the Concepts TS and C++20. In particular, only the name of a type-concept can be used to declare a template-parameter in the new rules:
template<typename> concept A = true; template<template<typename> typename> B = true; template<int> concept C = true; template<A> struct XA {}; // ok template<B> struct XB {}; // error, not a type-concept template<C> struct XC {}; // error, not a type-concept
We don't appear to have any explicit representation of the type-concept as written, only of its immediately-declared constraint. One design goal of Clang is for the AST to directly represent the program as written (to the extent possible) to facilitate writing tools on top of Clang's AST. Please consider adding a representation of the TypeConstraint itself. (This could be as simple as a wrapper around the Expr* that you currently have, that provides access to the concept name and the template arguments, and additionally stores a flag to determine whether an explicit template argument list was specified, to distinguish TypeConcept from TypeConcept<>.)
lib/Parse/ParseTemplate.cpp | ||
---|---|---|
590–619 ↗ | (On Diff #195801) | This is out of date compared to the current C++20 wording. |
677 ↗ | (On Diff #195801) | This should not require tentative parsing. We should be able to annotate the nested-name-specifier (if any) then determine whether we have a type-name or a type-constraint with a single token of lookahead. |
1039–1045 ↗ | (On Diff #195801) | This is incorrect; this rule was removed when concepts were merged into C++20. |
1153–1176 ↗ | (On Diff #195801) | All the work to build expressions and establish the semantic effects of declarations should be done by Sema, not by the Parser. The Parser should just figure out how the program matches the grammar, and report to Sema what it found. |
- removed old handling of constrained-parameters for type-constraints
- use template-id annotations during parsing to avoid tentative parses and set the ground for placeholder type-constraints
- add source information to the new TypeConstraint class, which shares a base class (ConceptReference) with ConceptSpecializationExpr
- change TemplateTypeParmDecl to not return true for wasDeclaredWithTypename if a type-constraint was present (and update places that assumed only 'class' or 'typename' were possible)
include/clang/AST/ASTNodeTraverser.h | ||
---|---|---|
518 ↗ | (On Diff #209698) | For consistency with DeclRefExpr, I think this function should be called hasExplicitTemplateArgs. |
include/clang/AST/RecursiveASTVisitor.h | ||
1775 | 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? | |
include/clang/Basic/DiagnosticSemaKinds.td | ||
2532 | "must not be" -> "is" | |
2536 | "must be" -> "is not" | |
lib/AST/ASTContext.cpp | ||
703–707 ↗ | (On Diff #209698) | 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.) |
709–716 ↗ | (On Diff #209698) | Similarly, we presumably shouldn't be preserving locations here through canonicalization. |
lib/AST/DeclPrinter.cpp | ||
1048–1049 ↗ | (On Diff #209698) | Nit: I think this would read more naturally if the type constraint case were first. |
lib/AST/TextNodeDumper.cpp | ||
1680–1682 ↗ | (On Diff #209698) | As above, I think this would be clearer if the "type constraint" and "typename" cases were reversed. |
1689 ↗ | (On Diff #209698) | It'd be useful to dump either the explicit arguments or the immediately-declared constraint here. |
lib/Parse/ParseExprCXX.cpp | ||
163–164 | We don't need both of these :) | |
486–489 | Is this necessary for this change in particular? (I'd expect isTemplateName to be able to cope with this corner case.) | |
lib/Parse/ParseTemplate.cpp | ||
552 ↗ | (On Diff #209698) | 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. |
619–644 ↗ | (On Diff #209698) | As mentioned above, I'd expect this all to be done by isStartOfTemplateTypeParameter itself. |
679–681 ↗ | (On Diff #209698) | 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. |
683–686 ↗ | (On Diff #209698) | This is out-of-date compared to the C++20 wording. |
801–811 ↗ | (On Diff #209698) | 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.) |
1286 ↗ | (On Diff #209698) | 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. |
lib/Sema/SemaCXXScopeSpec.cpp | ||
620 ↗ | (On Diff #209698) | Nit: && on the previous line, please. |
lib/Sema/SemaConcept.cpp | ||
844–847 | Please go ahead and commit this renaming (as a separate change). | |
lib/Sema/SemaTemplate.cpp | ||
1059 ↗ | (On Diff #209698) | 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?) |
1087 ↗ | (On Diff #209698) | This (and the later references to it) have the wrong section and paragraph listed: this is [temp.param]p4 not [temp]p6. |
1089 ↗ | (On Diff #209698) | 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). |
1123 ↗ | (On Diff #209698) | Similarly here. |
1967–1983 ↗ | (On Diff #209698) | 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). |
7047 ↗ | (On Diff #209698) | 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 :) |
lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
2326–2327 | As above, this should be recreated from scratch, not substituted. |
clang/include/clang/AST/DeclTemplate.h | ||
---|---|---|
1297–1300 ↗ | (On Diff #237228) | It'd be more efficient to ask if the immediately-declared constraint is a CXXFoldExpr, but it's a bit ugly to depend on that here. What do you think? |
clang/lib/Parse/ParseTemplate.cpp | ||
553 ↗ | (On Diff #237228) | I'm a bit confused by what's going on with this SuppressDiagnostic flag. This isn't a tentative parse; if we don't produce diagnostics for an invalid scope specifier here, I don't see anything that guarantees that any later code will produce any diagnostics either, which would be a problem. I would expect to be able to parse and produce diagnostics normally here, without this SuppressDiagnostic flag. |
635 ↗ | (On Diff #237228) | If isStartOfTemplateTypeParameter fails due to an invalid scope specifier, can we just return nullptr immediately, rather than making a likely-ill-fated attempt to parse a template or non-type parameter? It seems unlikely that we can reasonably recover in that case. |
676–677 ↗ | (On Diff #237228) | This is a bit surprising: the other bool TryAnnotate* functions follow the normal convention of returning true if they've encountered and diagnosed a parse error (and the caller is expected to check Tok to see whether it was annotated). Please consider making this consistent with the other similar functions. |
715–716 ↗ | (On Diff #237228) | I would expect it to fail if the template argument list is malformed, and in that case we should fail too. |
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
2484 ↗ | (On Diff #237228) | Nit: please add braces to this outer if. |
Remove SuppressDiagnostics, conform TryAnnotateTypeConstraint, readd concepts to relaxed template argument check, fix recovery from bad scope in ParseTemplateParameter
Some cleanups (mainly parameters that are no longer necessary), then this is good to go. Thanks!
clang/include/clang/Basic/LangOptions.def | ||
---|---|---|
146 ↗ | (On Diff #237478) | This change has no effect because the default value is unconditionally overwritten in CompilerInvocation.cpp. Revert for now; we can flip this flag as a separate change. |
clang/lib/AST/DeclTemplate.cpp | ||
110–114 ↗ | (On Diff #237478) | Do we need something like this for expanded template template parameter packs? |
clang/lib/Parse/ParseExprCXX.cpp | ||
2713 ↗ | (On Diff #237478) | This SuppressDiag parameter is never set by any caller; please remove it. |
clang/lib/Parse/ParseTemplate.cpp | ||
684 ↗ | (On Diff #237478) | ScopeError is always false here; you can remove the ErrorInTypeSpec parameter and the comment here. |
707 ↗ | (On Diff #237478) | The Tok.is check here is redundant; it's a subset of the following Tok.isNot check. |
clang/lib/Sema/SemaTemplate.cpp | ||
136 ↗ | (On Diff #237478) | This new parameter appears to be unused now. |
7088 ↗ | (On Diff #237478) | This is not appropriate; -fconcepts-ts (or hopefully soon -std=c++2a) should not override the value of this LangOpt. Please revert this change and manually turn on -frelaxed-template-template-args as necessary in the corresponding test cases. |
clang/lib/Sema/SemaTemplateDeduction.cpp | ||
645–659 ↗ | (On Diff #237478) | Looks like we repeat this (at least) three times. (And one of the instances is missing the template template parameter case.) Please consider moving this to somewhere more central (DeclTemplate.h?) -- as a separate patch, though. |
I saw a null pointer dereference for the p3-2a.cpp file added here. It is not always reproducible for me, I was able to reproduce it once offline. Any idea?
Crash stack is here:
https://results.llvm-merge-guard.org/amd64_debian_testing_clang8-1220/console-log.txt
From D67847, you could see that the first time merge bot is ran, p3-2a.cpp failed. The second time, without me doing anything except updating some unrelated tests, p3-2a.cpp passes.
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?