[PATCH] D11916: [CONCEPTS] Add diagnostic; invalid tag when concept specified

[Aaron Ballman via cfe-commits]

On Tue, Aug 11, 2015 at 3:36 PM, Nathan Wilson <nwilson20 at gmail.com> wrote:
>
>
> On Tue, Aug 11, 2015 at 8:54 AM, Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>>
>> On Mon, Aug 10, 2015 at 3:00 PM, Nathan Wilson <nwilson20 at gmail.com>
>> wrote:
>> > nwilson created this revision.
>> > nwilson added reviewers: rsmith, hubert.reinterpretcast, fraggamuffin,
>> > faisalv, aaron.ballman.
>> > nwilson added a subscriber: cfe-commits.
>> >
>> > Adding check to emit diagnostic for invalid tag when concept is
>> > specified and associated tests.
>> >
>> > http://reviews.llvm.org/D11916
>> >
>> > Files:
>> >   include/clang/Basic/DiagnosticSemaKinds.td
>> >   lib/Sema/SemaDecl.cpp
>> >   test/SemaCXX/cxx-concept-declaration.cpp
>> >
>> > Index: test/SemaCXX/cxx-concept-declaration.cpp
>> > ===================================================================
>> > --- test/SemaCXX/cxx-concept-declaration.cpp
>> > +++ test/SemaCXX/cxx-concept-declaration.cpp
>> > @@ -23,3 +23,13 @@
>> >  template<typename T>
>> >  concept bool D6; // expected-error {{variable concept declaration must
>> > be initialized}}
>> >
>> > +// Tag
>> > +concept class CC1 {}; // expected-error {{class cannot be marked
>> > concept}}
>> > +concept struct CS1 {}; // expected-error {{struct cannot be marked
>> > concept}}
>> > +concept union CU1 {}; // expected-error {{union cannot be marked
>> > concept}}
>> > +concept enum CE1 {}; // expected-error {{enum cannot be marked
>> > concept}}
>> > +template <typename T> concept class TCC1 {}; // expected-error {{class
>> > cannot be marked concept}}
>> > +template <typename T> concept struct TCS1 {}; // expected-error
>> > {{struct cannot be marked concept}}
>> > +template <typename T> concept union TCU1 {}; // expected-error {{union
>> > cannot be marked concept}}
>> > +
>> > +extern concept bool ExtC; // expected-error {{'concept' can only appear
>> > on the definition of a function template or variable template}}
>> > Index: lib/Sema/SemaDecl.cpp
>> > ===================================================================
>> > --- lib/Sema/SemaDecl.cpp
>> > +++ lib/Sema/SemaDecl.cpp
>> > @@ -3662,6 +3662,19 @@
>> >      return TagD;
>> >    }
>> >
>> > +  if (DS.isConceptSpecified()) {
>> > +    // C++ Concepts TS [dcl.spec.concept]p1: A concept definition
>> > refers to
>> > +    // either a function concept and its definition or a variable
>> > concept and
>> > +    // its initializer.
>> > +    if (Tag)
>> > +      Diag(DS.getConceptSpecLoc(), diag::err_concept_tag)
>> > +          << GetDiagnosticTypeSpecifierID(DS.getTypeSpecType());
>> > +    else
>> > +      Diag(DS.getConceptSpecLoc(),
>> > diag::err_concept_decl_non_template);
>> > +    // Don't emit warnings after this error.
>> > +    return TagD;
>> > +  }
>>
>> I'm not certain I understand why we need two different diagnostics for
>> this case. I think err_concept_decl_non_template is sufficiently clear
>> for both.
>>
>> ~Aaron
>
>
> This was based on how constexpr handles these checks.
>
> Perhaps someone can correct me if I'm wrong, but I believe the idea is that
> when the `struct` tag exists, for example, we think the user meant to
> declare a `struct` and not the start of a `concept` declaration. So, the
> `concept` specifier would be erroneous and we try give a more helpful
> diagnostic.

It could just be me, but I don't find the new diagnostic particularly
helpful. "foo cannot be marked concept" tells me why I have an error
but does not tell me what to do about it. "'concept' can only appear
on the definition of a function template or variable template" tells
me what I need to do to not have the error in the first place, as well
as why I have the error.

However, others may have differing opinions on the subject.

~Aaron

>
> I would need to add/fix the test case for this, but I tend to think the
> declaration such as `concept bool;` could be the users intention to try to
> create a `concept` declaration which is where the
> err_concept_decl_non_template comes in.
>
>>
>>
>> > +
>> >    DiagnoseFunctionSpecifiers(DS);
>> >
>> >    if (DS.isFriendSpecified()) {
>> > Index: include/clang/Basic/DiagnosticSemaKinds.td
>> > ===================================================================
>> > --- include/clang/Basic/DiagnosticSemaKinds.td
>> > +++ include/clang/Basic/DiagnosticSemaKinds.td
>> > @@ -1973,6 +1973,8 @@
>> >    "function concept declaration must be a definition">;
>> >  def err_var_concept_not_initialized : Error<
>> >    "variable concept declaration must be initialized">;
>> > +def err_concept_tag : Error<
>> > +  "%select{class|struct|interface|union|enum}0 cannot be marked
>> > concept">;
>> >
>> >  // C++11 char16_t/char32_t
>> >  def warn_cxx98_compat_unicode_type : Warning<
>> >
>> >
>
>