[PATCH] D40381: Parse concept definition

[Faisal Vali via Phabricator via cfe-commits]

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