[PATCH] D40381: Parse concept definition

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: include/clang/Sema/Sema.h:6194
+                                SourceLocation TemplateLoc,
+                                const TemplateArgumentListInfo *TemplateArgs);
+
----------------
changyu wrote:
> hubert.reinterpretcast wrote:
> > Indentation issue here too.
> That last line is 79 characters long.
clang-format is happy to give:
```
  ExprResult
  CheckConceptTemplateId(const CXXScopeSpec &SS,
                         const DeclarationNameInfo &NameInfo,
                         ConceptDecl *Template, SourceLocation TemplateLoc,
                         const TemplateArgumentListInfo *TemplateArgs);
```

I'm no fan of blindly using clang-format, but its output is sometimes useful.


================
Comment at: lib/Parse/ParseTemplate.cpp:374
+
+  ExprResult ConstraintExpr = ParseConstraintExpression();
+
----------------
changyu wrote:
> 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?
>From N4700 subclause 17.4.1.2 [temp.constr.atomic] paragraph 3:
[ ... ], and E shall be a constant expression of type bool

A search of "bool" in P0734R0 seems to indicate that is also the basis for the diagnostic Saar is requesting.

Although that wording only applies clearly when determining the satisfaction of C<T> for some T, it would be good to catch it early. I believe that the particular case I presented falls under the "no valid specialization" wording in [temp.res].

I think there is a gap between the wording and the intent if overloaded binary logical operators, detectable without substitution, are not sufficiently wrong on the part of the user that a compiler may refuse to translate the program.


================
Comment at: lib/Sema/SemaTemplate.cpp:7693
+Decl *Sema::ActOnConceptDefinition(Scope *S,
+                              MultiTemplateParamsArg TemplateParameterLists,
+                                   IdentifierInfo *Name, SourceLocation L,
----------------
changyu wrote:
> 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?
One workflow that works is to clang-format the base file, clang-format with your changes, grab a patch and then apply it to the original base file (probably needs some manual work).


https://reviews.llvm.org/D40381