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

[Nathan Wilson]

On Fri, Jul 10, 2015 at 8:52 AM, Hubert Tong <
hubert.reinterpretcast at gmail.com> wrote:

> On Fri, Jul 10, 2015 at 8:30 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>
>> Tiny nits, otherwise LGTM, but please wait for Richard to also sign off.
>>
> Same from my end.
>
>
>>
>> On Fri, Jul 10, 2015 at 12:21 AM, Nathan Wilson <nwilson20 at gmail.com>
>> wrote:
>> > nwilson updated this revision to Diff 29426.
>> > nwilson added a comment.
>> >
>> > Update phrasing for concept declaration scope diagnostic
>> > Fix indentation issue
>> > Update comments for function declaration being a definition
>> > Adding acceptance tests for concepts in namespace scope and adding
>> diagnostic test for variable concept not being in namespace scope
>> >
>> >
>> > http://reviews.llvm.org/D11027
>> >
>> > Files:
>> >   include/clang/Basic/DiagnosticSemaKinds.td
>> >   lib/Sema/SemaDecl.cpp
>> >   test/SemaCXX/cxx-concept-declaration.cpp
>> >
>> > Index: test/SemaCXX/cxx-concept-declaration.cpp
>> > ===================================================================
>> > --- /dev/null
>> > +++ test/SemaCXX/cxx-concept-declaration.cpp
>> > @@ -0,0 +1,17 @@
>> > +// RUN:  %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s
>> > +
>> > +namespace A {
>> > +  template<typename T> concept bool C1() { return true; }
>> > +
>> > +  template<typename T> concept bool C2 = true;
>> > +}
>> > +
>> > +template<typename T> concept bool D1(); // expected-error {{can only
>> declare a function concept with its definition}}
>> > +
>> > +struct B {
>> > +  template<typename T> concept bool D2() { return true; } //
>> expected-error {{concept declarations may only appear in namespace scope}}
>> > +};
>> > +
>> > +struct C {
>> > +  template<typename T> static concept bool D3 = true; //
>> expected-error {{concept declarations may only appear in namespace scope}}
>> > +};
>> > Index: lib/Sema/SemaDecl.cpp
>> > ===================================================================
>> > --- lib/Sema/SemaDecl.cpp
>> > +++ lib/Sema/SemaDecl.cpp
>> > @@ -4878,6 +4878,17 @@
>> >    if (getLangOpts().CPlusPlus)
>> >      CheckExtraCXXDefaultArguments(D);
>> >
>> > +  if (D.getDeclSpec().isConceptSpecified()) {
>> > +    // C++ Concepts TS [dcl.spec.concept]p1: The concept specifier
>> shall be
>> > +    // applied only to the definition of a function template or
>> variable
>> > +    // template, declared in namespace scope
>>
>> Missing a period.
>>
>> > +    if (!DC->getRedeclContext()->isFileContext()) {
>> > +      Diag(D.getIdentifierLoc(),
>> > +           diag::err_concept_decls_may_only_appear_in_namespace_scope);
>> > +      return nullptr;
>> > +    }
>> > +  }
>> > +
>> >    NamedDecl *New;
>> >
>> >    bool AddToScope = true;
>> > @@ -7223,6 +7234,7 @@
>> >      bool isVirtual = D.getDeclSpec().isVirtualSpecified();
>> >      bool isExplicit = D.getDeclSpec().isExplicitSpecified();
>> >      bool isConstexpr = D.getDeclSpec().isConstexprSpecified();
>> > +    bool isConcept = D.getDeclSpec().isConceptSpecified();
>> >      isFriend = D.getDeclSpec().isFriendSpecified();
>> >      if (isFriend && !isInline && D.isFunctionDefinition()) {
>> >        // C++ [class.friend]p5
>> > @@ -7439,6 +7451,21 @@
>> >          Diag(D.getDeclSpec().getConstexprSpecLoc(),
>> diag::err_constexpr_dtor);
>> >      }
>> >
>> > +    if (isConcept) {
>> > +      // C++ Concepts TS [dcl.spec.concept]p1: The concept specifier
>> shall be
>> > +      // applied only to the definition of a function template
>> > +      // (we are checking that the declaration is indeed a definition)
>>
>> Missing a period.
>>
> I think ellipsis is appropriate. Also, the parenthesized part is redundant
> now.
>

Yeah, I was attempting to be explicit, but I guess the code does that. I
can make the change.

>
>
>>
>> > +      if (!D.isFunctionDefinition()) {
>> > +        Diag(D.getDeclSpec().getConceptSpecLoc(),
>> > +             diag::err_function_concept_not_defined);
>> > +        NewFD->setInvalidDecl();
>> > +      }
>> > +
>> > +      // C++ Concepts TS [dcl.spec.concept]p2: Every concept
>> definition is
>> > +      // implicity defined to be a constexpr declaration (implicitly
>> inline)
>> > +      NewFD->setImplicitlyInline();
>> > +    }
>> > +
>> >      // If __module_private__ was specified, mark the function
>> accordingly.
>> >      if (D.getDeclSpec().isModulePrivateSpecified()) {
>> >        if (isFunctionTemplateSpecialization) {
>> > Index: include/clang/Basic/DiagnosticSemaKinds.td
>> > ===================================================================
>> > --- include/clang/Basic/DiagnosticSemaKinds.td
>> > +++ include/clang/Basic/DiagnosticSemaKinds.td
>> > @@ -1959,6 +1959,12 @@
>> >  def note_private_extern : Note<
>> >    "use __attribute__((visibility(\"hidden\"))) attribute instead">;
>> >
>> > +// C++ Concepts TS
>> > +def err_concept_decls_may_only_appear_in_namespace_scope : Error<
>> > +  "concept declarations may only appear in namespace scope">;
>> > +def err_function_concept_not_defined : Error<
>> > +  "can only declare a function concept with its definition">;
>>
>> This reads slightly strange to me. We have another diagnostic that we
>> could steal wording from:
>>
>> def err_invalid_constexpr_var_decl : Error<
>>   "constexpr variable declaration must be a definition">;
>>
>> So perhaps "function concept declaration must be a definition"?
>>
> That sounds good.
>

Okay, that works. Do you guys have any issue with the name:
err_function_concept_not_defined

Or, would something like this be more appropriate?:
err_invalid_concept_function_decl


>
>
>>
>> > +
>> >  // C++11 char16_t/char32_t
>> >  def warn_cxx98_compat_unicode_type : Warning<
>> >    "'%0' type specifier is incompatible with C++98">,
>>
>> ~Aaron
>>
>> >
>> >
>> >
>> > _______________________________________________
>> > 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/20150710/75b7dde4/attachment.html>