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

[Nathan Wilson via cfe-commits]

On Tue, Aug 11, 2015 at 5:46 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> On Tue, Aug 11, 2015 at 3:35 PM, Nathan Wilson <nwilson20 at gmail.com>
> wrote:
>
>>
>>
>> On Tue, Aug 11, 2015 at 4:54 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>>
>>> On Tue, Aug 11, 2015 at 2:46 PM, Nathan Wilson <nwilson20 at gmail.com>
>>> wrote:
>>>
>>>> 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?
>>>>
>>>
>>> Maybe err_concept_wrong_decl_kind?
>>>
>>
>> I'd be okay with that.
>>
>> Is okay to have two different tags with the same text?
>>
>> We'd use err_concept_decl_non_template in HandleDeclarator when we
>> encounter a non-template when `concept` is specified, and we could use err_concept_wrong_decl_kind
>> for the checks in this Patch.
>>
>
> Is there any reason to duplicate this rather than reusing the same
> diagnostic?
>

Well, I originally thought that the former was a bit more descriptive.
Nonetheless, I'd be fine with err_concept_wrong_decl_kind.


>
>
>> 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/787707cd/attachment-0001.html>