On Fri, Jul 10, 2015 at 9:44 AM, Aaron Ballman <aa...@aaronballman.com> wrote:
> 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. > Hmm, well, I'm leaning to what Hubert said about it being a bit too generic since there are other places where a name like this would be applicable (declared returning type not being bool, non empty parameter list, ...) > ~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