[PATCH] D40381: Parse concept definition
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 6 16:18:27 PDT 2018
rsmith added inline comments.
================
Comment at: include/clang/AST/DeclTemplate.h:3007
+/// \brief Definition of concept, not just declaration actually.
+class ConceptDecl : public TemplateDecl {
----------------
This comment isn't appropriate. Please just describe what the node is. (And note that a definition is a kind of declaration, despite common parlance.)
================
Comment at: include/clang/AST/DeclTemplate.h:3035
+ SourceRange getSourceRange() const override LLVM_READONLY {
+ return SourceRange(getLocation(), getLocation());
+ }
----------------
faisalv wrote:
> why not just fix it now?
> return SourceRange(getTemplateParameters()->getTemplateLoc(), ConstraintExpr->getLocEnd();
>
> ?
There's a bigger problem here:
```
TemplateDecl *TD = /*some ConceptDecl*/;
auto SR = TD->getSourceRange(); // crash
```
`TemplateDecl` has a hard assumption that it contains a `TemplatedDecl`. So, three options:
1) Change `TemplateDecl` and all its users to remove this assumption (hard and leads to an awkward-to-use AST)
2) Add a `Decl` subclass that acts as the templated declaration in a concept declaration (corresponding to the C++ grammar's //concept-definition// production)
3) Make `ConceptDecl` //not// derive from a `TemplateDecl` at all
I think option 2 is my preference, but option 3 is also somewhat appealing (there are other ways in which a concept is not a template: for instance, it cannot be constrained, and it cannot be classified as either a type template or a non-type template, because its kind depends on the context in which it appears).
Of course, this leads to one of the Hard Problems Of Computer Science: naming things. `ConceptDecl` for a //concept-definition// and `ConceptTemplateDecl` for the //template-head concept-definition// would be consistent with the rest of the AST. It's a little unfortunate for the longer name to be the AST node that we actually interact with, but the consistency is probably worth the cost.
================
Comment at: include/clang/AST/DeclTemplate.h:3027
+ }
+
+ // TODO: Should do source range properly.
----------------
saar.raz wrote:
> Add setConstraintExpr (for use when calling CreateDeserialized, see ASTDeclReader)
Please remove this again. `ASTDeclReader` should set the `ConstraintExpr` field directly. The AST is intended to be immutable after creation, so should generally not have setters.
================
Comment at: include/clang/AST/RecursiveASTVisitor.h:1728
+ TRY_TO(TraverseStmt(D->getConstraintExpr()));
+ // FIXME: Traverse all the concept specializations (one we implement forming template-ids with them).
+})
----------------
Hmm, concepts don't really have specializations, though, do they? (Much like alias templates.) And because they're substituted incrementally, and the result of evaluation can vary at different points in the same translation unit, it's not obvious how much we can actually cache.
I suppose I'll see this was handled in later patches in the series :)
================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1134
+def err_concept_unexpected_scope_spec : Error<
+ "concepts must be defined within their namespace">;
+}
----------------
I would expect something more general here. For an alias-declaration, we say:
"error: name defined in alias declaration must be an identifier"
This is then also appropriate for other kinds of invalid //concept-name//s such as
```
template<typename T> concept operator int = true;
```
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2390-2391
"%select{explicitly instantiated|explicitly specialized|partially specialized}1">;
+def err_concept_initialized_with_non_bool_type : Error<
+ "constraint expression must be 'bool' but found %0">;
+def err_concept_decls_may_only_appear_in_global_namespace_scope : Error<
----------------
"must be 'bool'" doesn't make sense. Maybe "constraint expression must be of type 'bool' but is of type %0" or similar?
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2394-2395
+ "concept declarations may only appear in global or namespace scope">;
+def err_concept_no_associated_constraints : Error<
+ "concept may not have associated constraints">;
+def err_concept_not_implemented : Error<
----------------
Do not use "may not" in this context; it's ambiguous (this could be read as "I'm not sure if this concept has associated constraints"). Use "cannot" instead.
(And generally prefer "can" over "may" or "must" in diagnostics.)
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2396-2397
+ "concept may not have associated constraints">;
+def err_concept_not_implemented : Error<
+ "this part's not implemented yet">;
+def err_concept_no_explicit_specialization : Error<
----------------
Please make this a bit less informal and a little more informative. Perhaps "sorry, unimplemented concepts feature used". For a temporary "under construction" error, it's also OK to include a %0 and pass in a string describing the feature if you like.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2398-2399
+ "this part's not implemented yet">;
+def err_concept_no_explicit_specialization : Error<
+ "concept maybe not be explicitly specialized">;
+
----------------
maybe not -> cannot
================
Comment at: include/clang/Basic/TemplateKinds.h:20
/// \brief Specifies the kind of template name that an identifier refers to.
/// Be careful when changing this: this enumeration is used in diagnostics.
enum TemplateNameKind {
----------------
Note this comment. You need to track down the diagnostics that use this enum and update their `%select`s to cover the new name kind.
================
Comment at: lib/AST/DeclBase.cpp:719-722
+
+ // Put concepts in tags namespace so that we get the following error:
+ // template<typename T> concept C5 = true;
+ // struct C5 {}; // expected-error {{redefinition}}
----------------
This comment just repeats what the comment before it already says: please remove it again.
================
Comment at: lib/Parse/ParseTemplate.cpp:161
// Parse the actual template declaration.
- return ParseSingleDeclarationAfterTemplate(Context,
- ParsedTemplateInfo(&ParamLists,
- isSpecialization,
- LastParamListWasEmpty),
- ParsingTemplateParams,
- DeclEnd, AS, AccessAttrs);
+ if (!TryConsumeToken(tok::kw_concept))
+ return ParseSingleDeclarationAfterTemplate(Context,
----------------
saar.raz wrote:
> Perhaps add a LangOpts.ConceptsTS check here?
No need. If you see a `kw_concept` keyword, you have the language feature enabled.
================
Comment at: lib/Parse/ParseTemplate.cpp:383-386
+ if (!Tok.is(tok::identifier)) {
+ Diag(Tok.getLocation(), diag::err_expected) << tok::identifier;
+ return nullptr;
+ }
----------------
saar.raz wrote:
> We could accept 'bool' here to be nice to people coming in from the old Concepts TS version of these decls - and emit a proper warning.
Please also check for a `<` after the identifier, and diagnose the attempt to form a concept partial / explicit specialization. (I'm fine with leaving that to a later patch if you prefer.)
One option would be to call `ParseUnqualifiedId` and then diagnose anything that's not a simple `identifier`. (See `ParseAliasDeclarationAfterDeclarator` for how we do this for alias declarations.)
================
Comment at: lib/Parse/ParseTemplate.cpp:392
+ if (!TryConsumeToken(tok::equal)) {
+ Diag(Tok.getLocation(), diag::err_expected) << "equal";
+ return nullptr;
----------------
Use `tok::equal` here, not a string literal.
================
Comment at: lib/Parse/ParseTemplate.cpp:399
+ Diag(Tok.getLocation(), diag::err_expected_expression)
+ << "constraint-expression";
+ return nullptr;
----------------
This diagnostic doesn't take an argument.
================
Comment at: lib/Sema/SemaTemplate.cpp:3922-3925
+ const bool DependentArguments =
+ TemplateSpecializationType::anyDependentTemplateArguments(
+ *TemplateArgs, InstantiationDependent);
+ if (R.getAsSingle<VarTemplateDecl>() && !DependentArguments) {
----------------
This needs rebasing onto SVN trunk. (We have lambda'fied the dependent arguments check there.)
================
Comment at: lib/Sema/SemaTemplate.cpp:7709
+ // scope.
+ if (!CurContext->isFileContext()) {
+ Diag(NameLoc,
----------------
You need to check `CurContext->getRedeclContext()`. A concept can be defined in a `LinkageSpecDecl` (with C++ linkage) or an `ExportDecl`, for instance.
================
Comment at: lib/Sema/SemaTemplate.cpp:7733-7734
+
+ if (!NewDecl)
+ return nullptr;
+
----------------
`ConceptDecl::Create` cannot return null. Checking this doesn't make sense, because (a) this is dead code, and (b) if this happened, you would return null without issuing a diagnostic.
https://reviews.llvm.org/D40381
More information about the cfe-commits
mailing list