[PATCH] D40381: Parse concept definition

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

rsmith accepted this revision.
rsmith added a comment.

LGTM with a few mechanical updates.



================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1262
+  "name defined in concept definition must be an identifier">;
+def err_concept_legacy_bool_keyword : ExtWarn<
+  "ISO C++2a does not permit the 'bool' keyword after 'concept'">,
----------------
`ExtWarn` diagnostics should be named `ext_` not `err_`. (This is important because readers of the code emitting the diagnostic need to know whether they can mark things as invalid (etc) during error recovery -- which is only correct to do after emitting an error.)


================
Comment at: include/clang/Sema/Sema.h:6676
+  // Concepts
+  Decl *ActOnConceptDefinition(Scope *S,
+      MultiTemplateParamsArg TemplateParameterLists, IdentifierInfo *Name,
----------------
Nit: please wrap the first parameter onto the next line.


================
Comment at: lib/AST/DeclTemplate.cpp:833-834
+                                 Expr *ConstraintExpr) {
+  // TODO: Do we need this?
+  //  AdoptTemplateParameterList(Params, cast<DeclContext>(Decl));
+  return new (C, DC) ConceptDecl(DC, L, Name, Params, ConstraintExpr);
----------------
Yes, you need that :)

(You should be able to check this with `-ast-dump`: for a concept in a namespace, its template parameters should have the namespace as their semantic `DeclContext`, not the translation unit. This also has some impact on merging of default argument visibility with modules.)


================
Comment at: lib/Index/IndexDecl.cpp:677
     if (D->getTemplateParameters() &&
-        shouldIndexTemplateParameterDefaultValue(Parent)) {
+        shouldIndexTemplateParameterDefaultValue(D, Parent)) {
       const TemplateParameterList *Params = D->getTemplateParameters();
----------------
Please revert the addition of the unused first parameter.


Repository:
  rC Clang

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

https://reviews.llvm.org/D40381