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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
