[PATCH] D40381: Parse concept definition

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks!

Please revert the (presumably unintended) mode changes to the `ptxas` executables.



================
Comment at: include/clang/AST/DeclTemplate.h:3035
+  SourceRange getSourceRange() const override LLVM_READONLY {
+    return SourceRange(getLocation(), getLocation());
+  }
----------------
saar.raz wrote:
> rsmith wrote:
> > 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.
> The dummy decl is pretty confusing and will be a very weird decl in and of itself. I can't think of a good name right now, but your proposed naming will be pretty confusing given that a ConceptTemplateDecl does not template a concept declaration given the meaning of the phrase in the standard... Maybe ConceptBodyDecl? ConceptDummyDecl? ConceptDefinitionDecl? We need something that screams "this is not interesting" for the AST usage to be reasonable. 
> Option 3 feels like loads of code duplication.
> I'm not entirely sure how many blind (through TemplateDecl) uses of getTemplatedDecl there are, but there might not be that many, in which case the first option might be the lesser of all these evils.
Faisal's comment is marked "Done" but not done.

`ConceptBodyDecl` or something like it seems reasonable. But I think we can consider that after landing this patch, and leave the templated declaration null for now.


================
Comment at: include/clang/AST/DeclTemplate.h:3016
+// \brief Declaration of a C++2a concept.
+class ConceptDecl : public TemplateDecl {
+protected:
----------------
This should also derive from `Mergeable<ConceptDecl>`, since we are permitted to merge multiple definitions of the same concept across translation units by C++20 [basic.def.odr]/12.


================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1260-1270
+
+def err_concept_definition_unexpected_scope_spec : Error<
+  "unexpected namespace scope before concept name; concepts must be defined "
+  "inside their namespace, if any">;
+
+def err_concept_definition_not_identifier : Error<
+  "name defined in concept definition must be an identifier">;
----------------
Generally we don't leave blank lines between diagnostic definitions, and instead use the continuation indent to visually separate them.


================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1261-1263
+def err_concept_definition_unexpected_scope_spec : Error<
+  "unexpected namespace scope before concept name; concepts must be defined "
+  "inside their namespace, if any">;
----------------
The phrasing of this (particularly the "if any") is a little confusing. I think it's fine to just use the `err_concept_definition_not_identifier` diagnostic for this case.


================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1269
+def err_concept_legacy_bool_keyword : Error<
+  "'bool' keyword after 'concept' is no longer valid in C++2a; remove it">;
+
----------------
I'd probably phrase this as

"ISO C++2a does not permit the 'bool' keyword after 'concept'"

(The Concepts TS isn't really the past -- TSes are more like an alternative reality -- so "no longer" is a bit odd.) I'd also be tempted to turn this into an `ExtWarn` so that we can accept code targeting the Concepts TS with a warning.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2443
+  "concept template parameter list must have at least one parameter; explicit "
+  "specialization of concepts is not allowed, did you attempt this?">;
+def err_concept_extra_headers : Error<
----------------
Drop the ", did you attempt this?". It doesn't add anything to the diagnostic, and might be read as a little snarky, frustrating the user.


================
Comment at: lib/AST/ASTDumper.cpp:182-186
+void ASTDumper::VisitConceptDecl(const ConceptDecl *D) {
+  NodeDumper.dumpName(D);
+  dumpTemplateParameters(D->getTemplateParameters());
+  Visit(D->getConstraintExpr());
+}
----------------
This needs rebasing; I believe the right place for this is now split between TextNodeDumper.cpp and ASTNodeTraverser.h.


================
Comment at: lib/AST/DeclPrinter.cpp:1084-1085
     Out << D->getName();
-  } else {
-    Visit(D->getTemplatedDecl());
-  }
+  } else if (auto *TD = D->getTemplatedDecl())
+    Visit(TD);
 }
----------------
This looks wrong: we'll only print the template parameters, and not the `concept <name> = <expr>;` part. Please add another `else if` for the `ConceptDecl` case (or similar).


================
Comment at: lib/Index/IndexDecl.cpp:658-662
+    if (isa<ConceptDecl>(TD))
+      // Concepts are not redeclarable.
+      return true;
+    if (!D)
+      return false;
----------------
This change looks redundant. Note that `VisitTemplateDecl` below already returned `true` if `getTemplatedDecl()` is null.


================
Comment at: lib/Parse/ParseTemplate.cpp:58
+///       template-head: [C++2a]
+///         'export'[opt] 'template' '<' template-parameter-list '>'
+///             requires-clause[opt]
----------------
There's no 'export'[opt] in //template-head//.


================
Comment at: lib/Parse/ParseTemplate.cpp:387
+
+  if (!TryConsumeToken(tok::equal)) {
+    Diag(Tok.getLocation(), diag::err_expected) << tok::equal;
----------------
Consider calling `DiagnoseAndSkipCXX11Attributes` before and after parsing the name of the concept; it seems likely that people will try adding attributes there, and we should give a good diagnostic for this until they start being permitted in one of those two locations.


================
Comment at: lib/Sema/SemaTemplate.cpp:7928
+  ActOnDocumentableDecl(NewDecl);
+  CurContext->addDecl(NewDecl);
+  return NewDecl;
----------------
This should be `PushOnScopeChains(NewDecl, S);` instead (though I think in practice it won't matter until/unless we start supporting block-scope concept definitions).


================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:3365
+  llvm_unreachable("Concept definitions cannot reside inside a template");
+  return nullptr;
+}
----------------
No need for a `return` here; the code after an `llvm_unreachable` is unreachable =)


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2082
+  D->ConstraintExpr = Record.readExpr();
+}
+
----------------
Call `mergeMergeable(D)` at the end of this.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40381/new/

https://reviews.llvm.org/D40381