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 > > >> >> > + >> > // 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