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

[Nathan Wilson via cfe-commits]

Okay, I'll make the change.

Hmm, do you guys have any suggestions as far as renaming
err_concept_decl_non_template?
How about err_concept_specifier_non_template?

We'd have to keep in mind that err_concept_decl_non_template is also used
outside of this check, i.e. `concept bool = true`.

On Tue, Aug 11, 2015 at 4:19 PM, Hubert Tong <
hubert.reinterpretcast at gmail.com> wrote:

> 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).
>
> -- HT
>
> On Tue, Aug 11, 2015 at 4:01 PM, Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>
>> 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<
>> >> >
>> >> >
>> >
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150811/36812003/attachment-0001.html>