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