On Tue, Aug 11, 2015 at 5:46 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
> On Tue, Aug 11, 2015 at 3:35 PM, Nathan Wilson <nwilso...@gmail.com> > wrote: > >> >> >> On Tue, Aug 11, 2015 at 4:54 PM, Richard Smith <rich...@metafoo.co.uk> >> wrote: >> >>> On Tue, Aug 11, 2015 at 2:46 PM, Nathan Wilson <nwilso...@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.reinterpretc...@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.ball...@gmail.com> wrote: >>>>> >>>>>> On Tue, Aug 11, 2015 at 3:36 PM, Nathan Wilson <nwilso...@gmail.com> >>>>>> wrote: >>>>>> > >>>>>> > >>>>>> > On Tue, Aug 11, 2015 at 8:54 AM, Aaron Ballman < >>>>>> aaron.ball...@gmail.com> >>>>>> > wrote: >>>>>> >> >>>>>> >> On Mon, Aug 10, 2015 at 3:00 PM, Nathan Wilson < >>>>>> nwilso...@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< >>>>>> >> > >>>>>> >> > >>>>>> > >>>>>> > >>>>>> >>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
