[PATCH] D40381: Parse concept definition

[Saar Raz via Phabricator via cfe-commits]

saar.raz added inline comments.
Herald added a subscriber: arphaman.


================
Comment at: include/clang/AST/DeclTemplate.h:3035
+  SourceRange getSourceRange() const override LLVM_READONLY {
+    return SourceRange(getLocation(), getLocation());
+  }
----------------
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.


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

https://reviews.llvm.org/D40381