faisalv added a comment.
Thanks for working on this! :)
================
Comment at: include/clang/AST/DeclTemplate.h:3035
+ SourceRange getSourceRange() const override LLVM_READONLY {
+ return SourceRange(getLocation(), getLocation());
+ }
----------------
why not just fix it now?
return SourceRange(getTemplateParameters()->getTemplateLoc(),
ConstraintExpr->getLocEnd();
?
================
Comment at: include/clang/AST/RecursiveASTVisitor.h:1725
+DEF_TRAVERSE_DECL(ConceptDecl, {})
+
----------------
Perhaps something along these lines?
DEF_TRAVERSE_DECL(ConceptDecl, {
TRY_TO(TraverseTemplateParameterListHelper(D->getTemplateParameters()));
TRY_TO(TraverseStmt(D->getConstraintExpr()));
// FIXME: Traverse all the concept specializations (one we implement
forming template-ids with them).
})
================
Comment at: include/clang/Sema/Sema.h:6196
+ const TemplateArgumentListInfo *TemplateArgs);
+
ExprResult BuildTemplateIdExpr(const CXXScopeSpec &SS,
----------------
Have you considered just deferring formation of concept template-ids to the
next patch (especially since it might require introduction of another AST node
ConceptSpecializationDecl. We could just focus on handling concept
declarations/definitions in this one (and emit an unimplemented error if
someone tries to form a template-id with a concept within BuildTemplateId etc.)
- but perhaps consider making sure we handle name clashes/redeclarations with
any other parsed names in the same namespace (within this patch)?
================
Comment at: lib/AST/DeclBase.cpp:773
+ return IDNS_Ordinary | IDNS_Tag;
+
// Never have names.
----------------
Pls consider just lumping this case with 'Binding/VarTemplate' (which by the
way also includes a comment for why we have to add the ugly IDNS_Tag hack here
(personally i think we should refactor this functionality, currently it seems
split between this and SemaLookup.cpp:getIDNS and would benefit from some sort
of well-designed cohesion - but that's another (low-priority) patch)
================
Comment at: lib/CodeGen/CGDecl.cpp:108
case Decl::Empty:
+ case Decl::Concept:
// None of these decls require codegen support.
----------------
You would need to add this to EmitTopLevelDecl too to handle global-namespace
concept declarations.
================
Comment at: lib/Parse/ParseTemplate.cpp:368
+
+ if (!TryConsumeToken(tok::equal)) {
+ Diag(Tok.getLocation(), diag::err_expected) << "equal";
----------------
I think we should diagnose qualified-name errors here much more gracefully than
we do.
i.e. template<class T> concept N::C = ...
- perhaps try and parse a scope-specifier, and if found emit a diagnostic such
as concepts must be defined within their namespace ...
================
Comment at: lib/Sema/SemaTemplate.cpp:3899
+ // constraint expressions right now.
+ return Template->getConstraintExpr();
+}
----------------
I don't think we want to just return the constraint expr? I think we would need
to create another conceptspecializationDecl node and nest it within a
DeclRefExpr? But once again, I think we should defer this to another patch -
no?
================
Comment at: lib/Sema/SemaTemplate.cpp:3923
bool InstantiationDependent;
- if (R.getAsSingle<VarTemplateDecl>() &&
- !TemplateSpecializationType::anyDependentTemplateArguments(
- *TemplateArgs, InstantiationDependent)) {
+ bool DependentArguments =
+ TemplateSpecializationType::anyDependentTemplateArguments(
----------------
const bool, no?
================
Comment at: lib/Sema/SemaTemplate.cpp:7692
+ MultiTemplateParamsArg TemplateParameterLists,
+ IdentifierInfo *Name, SourceLocation L,
+ Expr *ConstraintExpr) {
----------------
Rename L as NameLoc?
================
Comment at: lib/Sema/SemaTemplate.cpp:7706
+ }
+
+ ConceptDecl *NewDecl = ConceptDecl::Create(Context, DC, L, Name,
----------------
Perhaps add a lookup check here? (or a fixme that we are deferring to the next
patch), something along these *pseudo-code* lines:
LookupResult Previous(*this,
DeclarationNameInfo(DeclarationName(Name),NameLoc), LookupOrdinaryName);
LookupName(Previous, S);
if (Previous.getRepresentativeDecl()) ... if the decl is a concept - give
error regarding only one definition allowed in a TU, if a different kind of
decl then declared w a different symbol type of error?
================
Comment at: lib/Sema/SemaTemplate.cpp:7713
+
+ if (!ConstraintExpr->isTypeDependent() &&
+ ConstraintExpr->getType() != Context.BoolTy) {
----------------
Consider refactoring these checks on constraint expressions into a separate
function checkConstraintExpression (that we can also call from other contexts
such as requires-clauses and nested requires expressions)?
================
Comment at: lib/Sema/SemaTemplate.cpp:7735
+ ActOnDocumentableDecl(NewDecl);
+ CurContext->addDecl(NewDecl);
+ return NewDecl;
----------------
Why not use 'PushOnScopeChains' onto the enclosing scope?
Something along these lines:
assert(S->isTemplateParamScope() && S->getParent() &&
!S->getParent()->isTemplateParamScope() && "...");
PushOnScopeChains(NewDecl, S->getParent(),/*AddToContext*/true);
================
Comment at: test/Parser/cxx-concept-declaration.cpp:1
// Support parsing of concepts
----------------
Name this file starting with cxx2a- please.
https://reviews.llvm.org/D40381
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
