> 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
