On Fri, Jul 10, 2015 at 10:34 AM, Nathan Wilson <nwilso...@gmail.com> wrote: > > > On Fri, Jul 10, 2015 at 8:52 AM, Hubert Tong > <hubert.reinterpretc...@gmail.com> wrote: >> >> On Fri, Jul 10, 2015 at 8:30 AM, Aaron Ballman <aa...@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 <nwilso...@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
I prefer err_invalid_concept_function_decl. ~Aaron > >> >> >>> >>> >>> > + >>> > // 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@cs.uiuc.edu >>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> > >> >> > _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits