[PATCH] D40381: Parse concept definition

[changyu via Phabricator via cfe-commits]

changyu planned changes to this revision.
changyu marked 22 inline comments as done.
changyu added inline comments.


================
Comment at: lib/Parse/ParseTemplate.cpp:374
+
+  ExprResult ConstraintExpr = ParseConstraintExpression();
+
----------------
saar.raz wrote:
> Add a check to ParseConstraintExpression that the type is either dependent or bool, and add an apropriate diagnostic.
> 
> ```
>     if (!ConstraintExpr->isTypeDependent() && ConstraintExpr->getType() != Context.BoolTy) {
>       Diag(Init->getSourceRange().getBegin(),
>            diag::err_concept_initialized_with_non_bool_type) << Init->getType();
>       Concept->setInvalidDecl();
>       return;
>     }
> ```
I'm guessing you meant for this to be in `class Sema` so I added this to `Sema::ActOnConceptDefinition`. Also what is `Init` here?


================
Comment at: lib/Parse/ParseTemplate.cpp:374
+
+  ExprResult ConstraintExpr = ParseConstraintExpression();
+
----------------
hubert.reinterpretcast wrote:
> changyu wrote:
> > saar.raz wrote:
> > > Add a check to ParseConstraintExpression that the type is either dependent or bool, and add an apropriate diagnostic.
> > > 
> > > ```
> > >     if (!ConstraintExpr->isTypeDependent() && ConstraintExpr->getType() != Context.BoolTy) {
> > >       Diag(Init->getSourceRange().getBegin(),
> > >            diag::err_concept_initialized_with_non_bool_type) << Init->getType();
> > >       Concept->setInvalidDecl();
> > >       return;
> > >     }
> > > ```
> > I'm guessing you meant for this to be in `class Sema` so I added this to `Sema::ActOnConceptDefinition`. Also what is `Init` here?
> I think that would still need a TODO to instead walk the constraint expression for atomic constraints and diagnose those.
> ```
> template <typename T>
> concept C = 1 || T::value; // error
> ```
Why is that an error? [temp.constr.normal] in p0734r0 seems to say it's valid?


================
Comment at: lib/Sema/SemaTemplate.cpp:3903
+  //                                 /*FoundD=*/nullptr, TemplateArgs);
+  return Template->getConstraintExpr();
+  return true;
----------------
hubert.reinterpretcast wrote:
> Add more comments here. It looks like this allows us to get id-expressions naming concepts defined with non-dependent `bool` constraint expressions to "work" for now?
That's pretty much it.


================
Comment at: lib/Sema/SemaTemplate.cpp:3936
 
+  if (R.getAsSingle<ConceptDecl>()) {
+    return CheckConceptTemplateId(SS, R.getLookupNameInfo(),
----------------
Rakete1111 wrote:
> saar.raz wrote:
> > We're gonna want to check
> > ```
> > !TemplateSpecializationType::anyDependentTemplateArguments(*TemplateArgs, InstantiationDependent)
> > ``` 
> > here as well - so that we can instantiate a ConceptSpecializationExpr later when we have it (we're not gonna want to instantiate a ConceptSpecializationExpr with dependent arguments.
> No braces here please.
I'm following the style of the surrounding code here. I can remove the braces if you insist though.


================
Comment at: lib/Sema/SemaTemplate.cpp:7693
+Decl *Sema::ActOnConceptDefinition(Scope *S,
+                              MultiTemplateParamsArg TemplateParameterLists,
+                                   IdentifierInfo *Name, SourceLocation L,
----------------
Rakete1111 wrote:
> Did you run this through clang-format?
No, when I run the file through clang-format (with no arguments except the file), it reformats the whole file. How should I be running clang-format?


https://reviews.llvm.org/D40381