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

[Aaron Ballman]

On Thu, Jul 9, 2015 at 9:49 AM, Nathan Wilson <nwilson20 at gmail.com> wrote:
>
> 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 so late at night...

No worries!

>
> 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">;

I wonder if "concept declarations may only appear in namespace scope"
makes more sense, as there aren't any concept declarations other than
function and variable ones.

~Aaron

>
>> ~Aaron
>>
>> >
>> >>
>> >>
>> >> namespace A {
>> >>   template<typename T> concept bool D1() { return true; }
>> >> }
>> >>
>> >>
>> >> Either way, this is a test that should be added.
>> >
>> > Agreed.
>> >
>
> Makes sense. I'll add that.
>
>> >>
>> >>
>> >> ~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
>> >
>> >