<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 10, 2015 at 10:34 AM, Nathan Wilson <span dir="ltr"><<a href="mailto:nwilson20@gmail.com" target="_blank">nwilson20@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Fri, Jul 10, 2015 at 8:52 AM, Hubert Tong <span dir="ltr"><<a href="mailto:hubert.reinterpretcast@gmail.com" target="_blank">hubert.reinterpretcast@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On Fri, Jul 10, 2015 at 8:30 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Tiny nits, otherwise LGTM, but please wait for Richard to also sign off.<br></blockquote></span><div>Same from my end.<br></div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div><br>
On Fri, Jul 10, 2015 at 12:21 AM, Nathan Wilson <<a href="mailto:nwilson20@gmail.com" target="_blank">nwilson20@gmail.com</a>> wrote:<br>
> nwilson updated this revision to Diff 29426.<br>
> nwilson added a comment.<br>
><br>
> Update phrasing for concept declaration scope diagnostic<br>
> Fix indentation issue<br>
> Update comments for function declaration being a definition<br>
> Adding acceptance tests for concepts in namespace scope and adding diagnostic test for variable concept not being in namespace scope<br>
><br>
><br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D11027&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=WwvYPj5FR4621Yqsdp2M-YcEqfG27Le7SKGvz90jBaI&s=oe3P6lRJRGNNqBtQFklst3SxmxPwokPUG4HffhiFMCA&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11027</a><br>
><br>
> Files:<br>
> include/clang/Basic/DiagnosticSemaKinds.td<br>
> lib/Sema/SemaDecl.cpp<br>
> test/SemaCXX/cxx-concept-declaration.cpp<br>
><br>
> Index: test/SemaCXX/cxx-concept-declaration.cpp<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/SemaCXX/cxx-concept-declaration.cpp<br>
> @@ -0,0 +1,17 @@<br>
> +// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s<br>
> +<br>
> +namespace A {<br>
> + template<typename T> concept bool C1() { return true; }<br>
> +<br>
> + template<typename T> concept bool C2 = true;<br>
> +}<br>
> +<br>
> +template<typename T> concept bool D1(); // expected-error {{can only declare a function concept with its definition}}<br>
> +<br>
> +struct B {<br>
> + template<typename T> concept bool D2() { return true; } // expected-error {{concept declarations may only appear in namespace scope}}<br>
> +};<br>
> +<br>
> +struct C {<br>
> + template<typename T> static concept bool D3 = true; // expected-error {{concept declarations may only appear in namespace scope}}<br>
> +};<br>
> Index: lib/Sema/SemaDecl.cpp<br>
> ===================================================================<br>
> --- lib/Sema/SemaDecl.cpp<br>
> +++ lib/Sema/SemaDecl.cpp<br>
> @@ -4878,6 +4878,17 @@<br>
> if (getLangOpts().CPlusPlus)<br>
> CheckExtraCXXDefaultArguments(D);<br>
><br>
> + if (D.getDeclSpec().isConceptSpecified()) {<br>
> + // C++ Concepts TS [dcl.spec.concept]p1: The concept specifier shall be<br>
> + // applied only to the definition of a function template or variable<br>
> + // template, declared in namespace scope<br>
<br>
</div></div>Missing a period.<br>
<span><br>
> + if (!DC->getRedeclContext()->isFileContext()) {<br>
> + Diag(D.getIdentifierLoc(),<br>
> + diag::err_concept_decls_may_only_appear_in_namespace_scope);<br>
> + return nullptr;<br>
> + }<br>
> + }<br>
> +<br>
> NamedDecl *New;<br>
><br>
> bool AddToScope = true;<br>
> @@ -7223,6 +7234,7 @@<br>
> bool isVirtual = D.getDeclSpec().isVirtualSpecified();<br>
> bool isExplicit = D.getDeclSpec().isExplicitSpecified();<br>
> bool isConstexpr = D.getDeclSpec().isConstexprSpecified();<br>
> + bool isConcept = D.getDeclSpec().isConceptSpecified();<br>
> isFriend = D.getDeclSpec().isFriendSpecified();<br>
> if (isFriend && !isInline && D.isFunctionDefinition()) {<br>
> // C++ [class.friend]p5<br>
> @@ -7439,6 +7451,21 @@<br>
> Diag(D.getDeclSpec().getConstexprSpecLoc(), diag::err_constexpr_dtor);<br>
> }<br>
><br>
> + if (isConcept) {<br>
> + // C++ Concepts TS [dcl.spec.concept]p1: The concept specifier shall be<br>
> + // applied only to the definition of a function template<br>
> + // (we are checking that the declaration is indeed a definition)<br>
<br>
</span>Missing a period.<br></blockquote></div></div><div>I think ellipsis is appropriate. Also, the parenthesized part is redundant now.<br></div></div></div></div></blockquote><div><br></div></div></div><div>Yeah, I was attempting to be explicit, but I guess the code does that. I can make the change. </div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div><br>
> + if (!D.isFunctionDefinition()) {<br>
> + Diag(D.getDeclSpec().getConceptSpecLoc(),<br>
> + diag::err_function_concept_not_defined);<br>
> + NewFD->setInvalidDecl();<br>
> + }<br>
> +<br>
> + // C++ Concepts TS [dcl.spec.concept]p2: Every concept definition is<br>
> + // implicity defined to be a constexpr declaration (implicitly inline)<br>
> + NewFD->setImplicitlyInline();<br>
> + }<br>
> +<br>
> // If __module_private__ was specified, mark the function accordingly.<br>
> if (D.getDeclSpec().isModulePrivateSpecified()) {<br>
> if (isFunctionTemplateSpecialization) {<br>
> Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
> ===================================================================<br>
> --- include/clang/Basic/DiagnosticSemaKinds.td<br>
> +++ include/clang/Basic/DiagnosticSemaKinds.td<br>
> @@ -1959,6 +1959,12 @@<br>
> def note_private_extern : Note<<br>
> "use __attribute__((visibility(\"hidden\"))) attribute instead">;<br>
><br>
> +// C++ Concepts TS<br>
> +def err_concept_decls_may_only_appear_in_namespace_scope : Error<<br>
> + "concept declarations may only appear in namespace scope">;<br>
> +def err_function_concept_not_defined : Error<<br>
> + "can only declare a function concept with its definition">;<br>
<br>
</div></div>This reads slightly strange to me. We have another diagnostic that we<br>
could steal wording from:<br>
<br>
def err_invalid_constexpr_var_decl : Error<<br>
"constexpr variable declaration must be a definition">;<br>
<br>
So perhaps "function concept declaration must be a definition"?<br></blockquote></div></div><div>That sounds good.<br></div></div></div></div></blockquote><div><br></div></div></div><div>Okay, that works. Do you guys have any issue with the name:</div><div>err_function_concept_not_defined<br></div></div></div></div></blockquote><div>I like this name better than the one below (which is a bit too generic).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><br></div><div>Or, would something like this be more appropriate?:</div><div>err_invalid_concept_function_decl</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><br>
> +<br>
> // C++11 char16_t/char32_t<br>
> def warn_cxx98_compat_unicode_type : Warning<<br>
> "'%0' type specifier is incompatible with C++98">,<br>
<br>
</span><span><font color="#888888">~Aaron<br>
</font></span><div><div><br>
><br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
</div></div></blockquote></span></div><br></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>