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? > 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
