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