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

[Hubert Tong]

On Fri, Jul 10, 2015 at 10:34 AM, Nathan Wilson <nwilson20 at gmail.com> wrote:

>
>
> 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
>
I like this name better than the one below (which is a bit too generic).


>
> 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/349faec6/attachment.html>