> On Oct 11, 2017, at 16:02, Richard Smith <rich...@metafoo.co.uk> wrote: > > On 22 September 2017 at 18:00, Volodymyr Sapsai via cfe-commits > <cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>> wrote: > > >> On Sep 21, 2017, at 15:17, Richard Smith <rich...@metafoo.co.uk >> <mailto:rich...@metafoo.co.uk>> wrote: >> >> On 15 September 2017 at 12:51, Volodymyr Sapsai via cfe-commits >> <cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>> wrote: >> Author: vsapsai >> Date: Fri Sep 15 12:51:42 2017 >> New Revision: 313386 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=313386&view=rev >> <http://llvm.org/viewvc/llvm-project?rev=313386&view=rev> >> Log: >> [Sema] Error out early for tags defined inside an enumeration. >> >> This fixes PR28903 by avoiding access check for inner enum constant. We >> are performing access check because one enum constant references another >> and because enum is defined in CXXRecordDecl. But access check doesn't >> work because FindDeclaringClass doesn't expect more than one EnumDecl >> and because inner enum has access AS_none due to not being an immediate >> child of a record. >> >> The change detects an enum is defined in wrong place and allows to skip >> parsing its body. Access check is skipped together with body parsing. >> There was no crash in C, added test case to cover the new error. >> >> rdar://problem/28530809 <> >> >> Reviewers: rnk, doug.gregor, rsmith >> >> Reviewed By: doug.gregor >> >> Subscribers: cfe-commits >> >> Differential Revision: https://reviews.llvm.org/D37089 >> <https://reviews.llvm.org/D37089> >> >> >> Modified: >> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >> cfe/trunk/lib/Sema/SemaDecl.cpp >> cfe/trunk/test/Sema/enum.c >> cfe/trunk/test/SemaCXX/enum.cpp >> >> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=313386&r1=313385&r2=313386&view=diff >> >> <http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=313386&r1=313385&r2=313386&view=diff> >> ============================================================================== >> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) >> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Sep 15 12:51:42 >> 2017 >> @@ -1335,6 +1335,8 @@ def err_type_defined_in_alias_template : >> "%0 cannot be defined in a type alias template">; >> def err_type_defined_in_condition : Error< >> "%0 cannot be defined in a condition">; >> +def err_type_defined_in_enum : Error< >> + "%0 cannot be defined in an enumeration">; >> >> def note_pure_virtual_function : Note< >> "unimplemented pure virtual method %0 in %1">; >> >> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=313386&r1=313385&r2=313386&view=diff >> >> <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=313386&r1=313385&r2=313386&view=diff> >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Sep 15 12:51:42 2017 >> @@ -13928,6 +13928,12 @@ CreateNewDecl: >> Invalid = true; >> } >> >> + if (!Invalid && TUK == TUK_Definition && DC->getDeclKind() == Decl::Enum) >> { >> + Diag(New->getLocation(), diag::err_type_defined_in_enum) >> + << Context.getTagDeclType(New); >> + Invalid = true; >> + } >> >> This looks like the wrong fix. As noted elsewhere, this is wrong in C. And >> in C++, the relevant context is a type-specifier, which should be rejected >> due to the check 7 lines above. >> >> It looks like the actual bug is that we don't consider the type within a C99 >> compound literal to be a type-specifier. The fact that the context is an >> enumeration is irrelevant. > > At which point can we detect IsTypeSpecifier should be true? Which in turn > boils down to DeclSpecContext should be DSC_type_specifier. Currently we have > DeclSpecContext DSC_normal because it is a default argument in > Parser::ParseSpecifierQualifierList. Which is called from > > #4 clang::Parser::ParseParenExpression(clang::Parser::ParenParseOption&, > bool, bool, clang::OpaquePtr<clang::QualType>&, clang::SourceLocation&) at > llvm-project/clang/lib/Parse/ParseExpr.cpp:2375 > > The call to ParseSpecifierQualfiierList here should always pass > DSC_type_specifier. We're parsing a type within parentheses (which we've > already disambiguated as being a type cast / compound literal rather than an > expression), which is the DSC_type_specifier case. Thanks, Richard, that works. As the result in clang/test/CXX/drs/dr5xx.cpp for
sizeof(const); We change error from "requires a type specifier” to “expected a type” which seems OK to me (we use it for other dr539 cases). Will send patch for code review soon. > #5 clang::Parser::ParseCastExpression(bool, bool, bool&, > clang::Parser::TypeCastState, bool) at > llvm-project/clang/lib/Parse/ParseExpr.cpp:768 > #6 clang::Parser::ParseCastExpression(bool, bool, > clang::Parser::TypeCastState, bool) at > llvm-project/clang/lib/Parse/ParseExpr.cpp:521 > #7 > clang::Parser::ParseConstantExpressionInExprEvalContext(clang::Parser::TypeCastState) > at llvm-project/clang/lib/Parse/ParseExpr.cpp:201 > > I have considered using TypeCastState for setting DeclSpecContext but its > value is NotTypeCast because Parser::ParseEnumBody calls > ParseConstantExpression with default argument. And it looks correct as > parsing enum body doesn't imply presence of a type cast. > > I was struggling to find a good indication we are parsing type specifier and > the best option seems to be ParseCastExpression because it expects a type. > But it is too broad and likely to cause false positives. In quick prototype > it didn't work so I didn't pursue it further. Do you think it is possible to > tell we are in type specifier based on tokens we parsed? Specifically "(" > seems to be significant as without it parsing goes in different direction. > >> >> + >> // Maybe add qualifier info. >> if (SS.isNotEmpty()) { >> if (SS.isSet()) { >> >> Modified: cfe/trunk/test/Sema/enum.c >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/enum.c?rev=313386&r1=313385&r2=313386&view=diff >> >> <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/enum.c?rev=313386&r1=313385&r2=313386&view=diff> >> ============================================================================== >> --- cfe/trunk/test/Sema/enum.c (original) >> +++ cfe/trunk/test/Sema/enum.c Fri Sep 15 12:51:42 2017 >> @@ -123,3 +123,14 @@ int NegativeShortTest[NegativeShort == - >> // PR24610 >> enum Color { Red, Green, Blue }; // expected-note{{previous use is here}} >> typedef struct Color NewColor; // expected-error {{use of 'Color' with tag >> type that does not match previous declaration}} >> + >> +// PR28903 >> +struct PR28903 { >> + enum { >> + PR28903_A = (enum { // expected-error-re {{'enum PR28903::(anonymous at >> {{.*}})' cannot be defined in an enumeration}} >> + PR28903_B, >> + PR28903_C = PR28903_B >> + })0 >> + }; >> + int makeStructNonEmpty; >> +}; >> >> Modified: cfe/trunk/test/SemaCXX/enum.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/enum.cpp?rev=313386&r1=313385&r2=313386&view=diff >> >> <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/enum.cpp?rev=313386&r1=313385&r2=313386&view=diff> >> ============================================================================== >> --- cfe/trunk/test/SemaCXX/enum.cpp (original) >> +++ cfe/trunk/test/SemaCXX/enum.cpp Fri Sep 15 12:51:42 2017 >> @@ -110,3 +110,13 @@ enum { overflow = 123456 * 234567 }; >> // expected-warning@-2 {{not an integral constant expression}} >> // expected-note@-3 {{value 28958703552 is outside the range of >> representable values}} >> #endif >> + >> +// PR28903 >> +struct PR28903 { >> + enum { >> + PR28903_A = (enum { // expected-error-re {{'PR28903::(anonymous enum at >> {{.*}})' cannot be defined in an enumeration}} >> + PR28903_B, >> + PR28903_C = PR28903_B >> + }) >> + }; >> +}; >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits> > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits