<div dir="ltr">Okay, I'll make the change.<div><br></div><div>Hmm, do you guys have any suggestions as far as renaming <span style="font-size:12.8000001907349px">err_concept_decl_non_template? How about </span><span style="font-size:12.8000001907349px">err_concept_specifier_non_template?</span></div><div><span style="font-size:12.8000001907349px"><br></span></div><div><span style="font-size:12.8000001907349px">We'd have to keep in mind that </span><span style="font-size:12.8000001907349px">err_concept_decl_non_template is also used outside of this check, i.e. `concept bool = true`.</span><span style="font-size:12.8000001907349px"><br></span><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 11, 2015 at 4:19 PM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>From Aaron's description of the user experience, I think the err_concept_decl_non_template message text is good (although err_concept_decl_non_template might need to be renamed).<span class="HOEnZb"><font color="#888888"><br></font></span></div><span class="HOEnZb"><font color="#888888"><br></font></span></div><span class="HOEnZb"><font color="#888888">-- HT<br></font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 11, 2015 at 4:01 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@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><div>On Tue, Aug 11, 2015 at 3:36 PM, Nathan Wilson <<a href="mailto:nwilson20@gmail.com" target="_blank">nwilson20@gmail.com</a>> wrote:<br>
><br>
><br>
> On Tue, Aug 11, 2015 at 8:54 AM, Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a>><br>
> wrote:<br>
>><br>
>> On Mon, Aug 10, 2015 at 3:00 PM, Nathan Wilson <<a href="mailto:nwilson20@gmail.com" target="_blank">nwilson20@gmail.com</a>><br>
>> wrote:<br>
>> > nwilson created this revision.<br>
>> > nwilson added reviewers: rsmith, hubert.reinterpretcast, fraggamuffin,<br>
>> > faisalv, aaron.ballman.<br>
>> > nwilson added a subscriber: cfe-commits.<br>
>> ><br>
>> > Adding check to emit diagnostic for invalid tag when concept is<br>
>> > specified and associated tests.<br>
>> ><br>
>> > <a href="http://reviews.llvm.org/D11916" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11916</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>
>> > --- test/SemaCXX/cxx-concept-declaration.cpp<br>
>> > +++ test/SemaCXX/cxx-concept-declaration.cpp<br>
>> > @@ -23,3 +23,13 @@<br>
>> >  template<typename T><br>
>> >  concept bool D6; // expected-error {{variable concept declaration must<br>
>> > be initialized}}<br>
>> ><br>
>> > +// Tag<br>
>> > +concept class CC1 {}; // expected-error {{class cannot be marked<br>
>> > concept}}<br>
>> > +concept struct CS1 {}; // expected-error {{struct cannot be marked<br>
>> > concept}}<br>
>> > +concept union CU1 {}; // expected-error {{union cannot be marked<br>
>> > concept}}<br>
>> > +concept enum CE1 {}; // expected-error {{enum cannot be marked<br>
>> > concept}}<br>
>> > +template <typename T> concept class TCC1 {}; // expected-error {{class<br>
>> > cannot be marked concept}}<br>
>> > +template <typename T> concept struct TCS1 {}; // expected-error<br>
>> > {{struct cannot be marked concept}}<br>
>> > +template <typename T> concept union TCU1 {}; // expected-error {{union<br>
>> > cannot be marked concept}}<br>
>> > +<br>
>> > +extern concept bool ExtC; // expected-error {{'concept' can only appear<br>
>> > on the definition of a function template or variable template}}<br>
>> > Index: lib/Sema/SemaDecl.cpp<br>
>> > ===================================================================<br>
>> > --- lib/Sema/SemaDecl.cpp<br>
>> > +++ lib/Sema/SemaDecl.cpp<br>
>> > @@ -3662,6 +3662,19 @@<br>
>> >      return TagD;<br>
>> >    }<br>
>> ><br>
>> > +  if (DS.isConceptSpecified()) {<br>
>> > +    // C++ Concepts TS [dcl.spec.concept]p1: A concept definition<br>
>> > refers to<br>
>> > +    // either a function concept and its definition or a variable<br>
>> > concept and<br>
>> > +    // its initializer.<br>
>> > +    if (Tag)<br>
>> > +      Diag(DS.getConceptSpecLoc(), diag::err_concept_tag)<br>
>> > +          << GetDiagnosticTypeSpecifierID(DS.getTypeSpecType());<br>
>> > +    else<br>
>> > +      Diag(DS.getConceptSpecLoc(),<br>
>> > diag::err_concept_decl_non_template);<br>
>> > +    // Don't emit warnings after this error.<br>
>> > +    return TagD;<br>
>> > +  }<br>
>><br>
>> I'm not certain I understand why we need two different diagnostics for<br>
>> this case. I think err_concept_decl_non_template is sufficiently clear<br>
>> for both.<br>
>><br>
>> ~Aaron<br>
><br>
><br>
> This was based on how constexpr handles these checks.<br>
><br>
> Perhaps someone can correct me if I'm wrong, but I believe the idea is that<br>
> when the `struct` tag exists, for example, we think the user meant to<br>
> declare a `struct` and not the start of a `concept` declaration. So, the<br>
> `concept` specifier would be erroneous and we try give a more helpful<br>
> diagnostic.<br>
<br>
</div></div>It could just be me, but I don't find the new diagnostic particularly<br>
helpful. "foo cannot be marked concept" tells me why I have an error<br>
but does not tell me what to do about it. "'concept' can only appear<br>
on the definition of a function template or variable template" tells<br>
me what I need to do to not have the error in the first place, as well<br>
as why I have the error.<br>
<br>
However, others may have differing opinions on the subject.<br>
<span><font color="#888888"><br>
~Aaron<br>
</font></span><div><div><br>
><br>
> I would need to add/fix the test case for this, but I tend to think the<br>
> declaration such as `concept bool;` could be the users intention to try to<br>
> create a `concept` declaration which is where the<br>
> err_concept_decl_non_template comes in.<br>
><br>
>><br>
>><br>
>> > +<br>
>> >    DiagnoseFunctionSpecifiers(DS);<br>
>> ><br>
>> >    if (DS.isFriendSpecified()) {<br>
>> > Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
>> > ===================================================================<br>
>> > --- include/clang/Basic/DiagnosticSemaKinds.td<br>
>> > +++ include/clang/Basic/DiagnosticSemaKinds.td<br>
>> > @@ -1973,6 +1973,8 @@<br>
>> >    "function concept declaration must be a definition">;<br>
>> >  def err_var_concept_not_initialized : Error<<br>
>> >    "variable concept declaration must be initialized">;<br>
>> > +def err_concept_tag : Error<<br>
>> > +  "%select{class|struct|interface|union|enum}0 cannot be marked<br>
>> > concept">;<br>
>> ><br>
>> >  // C++11 char16_t/char32_t<br>
>> >  def warn_cxx98_compat_unicode_type : Warning<<br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div>