[PATCH] D11027: [CONCEPTS] Creating Diagnostics for ill-formed function concept declaration

[Nathan Wilson]

On Jul 9, 2015 8:21 AM, "Aaron Ballman" <aaron at aaronballman.com> wrote:
>
> On Thu, Jul 9, 2015 at 9:17 AM, Hubert Tong
> <hubert.reinterpretcast at gmail.com> wrote:
> > On Thu, Jul 9, 2015 at 7:52 AM, Aaron Ballman <aaron at aaronballman.com>
> > wrote:
> >>
> >> On Thu, Jul 9, 2015 at 12:22 AM, Hubert Tong
> >> <hubert.reinterpretcast at gmail.com> wrote:
> >> > hubert.reinterpretcast added inline comments.
> >> >
> >> > ================
> >> > Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1964
> >> > @@ +1963,3 @@
> >> > +def err_invalid_concept_scope : Error<
> >> > +  "concept declaration required to be in namespace scope">;
> >> > +def err_function_concept_not_defined : Error<
> >> > ----------------
> >> > This error is similar to
> >> > `err_objc_decls_may_only_appear_in_global_scope` below, so perhaps
it could
> >> > be expressed in a similar manner.
> >>
> >> Can't concepts be declared in a namespace scope that's not the global
> >> namespace scope?
> >
> > Yes. I should have been more specific. The similarity I was referring
to did
> > not extend to the "global scope" part. Note that I added my comment to
the
> > message text (not the check, which does allow namespaces other than the
> > global one).
>
> I mentioned it because the current patch has:
>
> +def err_concept_decls_may_only_appear_in_global_scope : Error<
> +  "function and variable concept declarations may only appear in
> global scope">;
>
> which is a misleading diagnostic (both the identifier and the
> wording). I probably should have attributed that to the current patch
> and not your comment, however. ;-)

Yeah, sorry, probably shouldn't have tried to do it so late at night...

Would this wording suffice?:

def err_concept_decls_may_only_appear_in_namespace_scope : Error<
"function and variable concept declarations may only appear in namespace
scope">;

>
> ~Aaron
>
> >
> >>
> >>
> >> namespace A {
> >>   template<typename T> concept bool D1() { return true; }
> >> }
> >>

Makes sense. I'll add that. Thanks!

> >>
> >> Either way, this is a test that should be added.
> >
> > Agreed.
> >
> >>
> >>
> >> ~Aaron
> >>
> >> >
> >> > ================
> >> > Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1966
> >> > @@ +1965,3 @@
> >> > +def err_function_concept_not_defined : Error<
> >> > +  "function concept requires definition">;
> >> > +
> >> > ----------------
> >> > Perhaps: "can only declare a function concept with its definition".
> >> >
> >> > The existing wording seems to imply that an earlier definition (of
just
> >> > one somewhere) is sufficient and that function concepts may be
redeclared.
> >> >
> >> > ================
> >> > Comment at: lib/Sema/SemaDecl.cpp:7453
> >> > @@ +7452,3 @@
> >> > +      // C++ Concepts TS [dcl.spec.concept]p1: A concept definition
> >> > refers to a
> >> > +      // function concept and its definition
> >> > +      if (!D.isFunctionDefinition()) {
> >> > ----------------
> >> > The quoted text is a definition of the term "concept definition". The
> >> > diagnosable rule is established by the quote on line 7444. So there
probably
> >> > should be a comment here to the effect of "we are checking that the
> >> > declaration is indeed a definition".
> >> >
> >> > ================
> >> > Comment at: lib/Sema/SemaDecl.cpp:7460
> >> > @@ +7459,3 @@
> >> > +
> >> > +      // C++ Concepts TS [dcl.spec.concept]p2: Every concepts
> >> > definition is
> >> > +      // implicity defined to be a constexpr declaration (implicitly
> >> > inline)
> >> > ----------------
> >> > Typo: s/concepts/concept/
> >> >
> >> > ================
> >> > Comment at: test/SemaCXX/cxx-concept-declaration.cpp:3
> >> > @@ +2,3 @@
> >> > +
> >> > +template<typename T> concept bool C1 = true;
> >> > +
> >> > ----------------
> >> > I'm not sure why this is part of the current patch.
> >> >
> >> > ================
> >> > Comment at: test/SemaCXX/cxx-concept-declaration.cpp:5
> >> > @@ +4,3 @@
> >> > +
> >> > +template<typename T> concept decltype(!0) C2 = true;
> >> > +
> >> > ----------------
> >> > Same comment as on line 3.
> >> >
> >> >
> >> > http://reviews.llvm.org/D11027
> >> >
> >> >
> >> >
> >> >
> >> > _______________________________________________
> >> > cfe-commits mailing list
> >> > cfe-commits at cs.uiuc.edu
> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150709/206e0394/attachment.html>