Message ID | ca1511be-8e53-05a1-c3e2-fcf40f7e8329@oracle.com |
---|---|
State | New |
Headers | show |
Series | [C++] One more DECL_SOURCE_LOCATION | expand |
On Fri, Oct 11, 2019 at 12:58:41PM +0200, Paolo Carlini wrote: > Hi, > > another straightforward DECL_SOURCE_LOCATION (TYPE_MAIN_DECL consistent with > the one I used in build_anon_union_vars. Tested x86_64-linux. > > Thanks, Paolo. > > ///////////////////// > /cp > 2019-10-11 Paolo Carlini <paolo.carlini@oracle.com> > > * decl.c (check_tag_decl): Use DECL_SOURCE_LOCATION. > > /testsuite > 2019-10-11 Paolo Carlini <paolo.carlini@oracle.com> > > * g++.dg/cpp0x/constexpr-union5.C: Test location(s) too. > * g++.dg/diagnostic/bitfld2.C: Likewise. > * g++.dg/ext/anon-struct1.C: Likewise. > * g++.dg/ext/anon-struct6.C: Likewise. > * g++.dg/ext/flexary19.C: Likewise. > * g++.dg/ext/flexary9.C: Likewise. > * g++.dg/template/error17.C: Likewise. > Index: cp/decl.c > =================================================================== > --- cp/decl.c (revision 276845) > +++ cp/decl.c (working copy) > @@ -4992,7 +4992,8 @@ check_tag_decl (cp_decl_specifier_seq *declspecs, > > if (TREE_CODE (declared_type) != UNION_TYPE > && !in_system_header_at (input_location)) > - pedwarn (input_location, OPT_Wpedantic, "ISO C++ prohibits anonymous structs"); > + pedwarn (DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (declared_type)), > + OPT_Wpedantic, "ISO C++ prohibits anonymous structs"); The change looks fine but the in_system_header_at line can be dropped, right? Marek
Hi, On 11/10/19 15:37, Marek Polacek wrote: > >> Index: cp/decl.c >> =================================================================== >> --- cp/decl.c (revision 276845) >> +++ cp/decl.c (working copy) >> @@ -4992,7 +4992,8 @@ check_tag_decl (cp_decl_specifier_seq *declspecs, >> >> if (TREE_CODE (declared_type) != UNION_TYPE >> && !in_system_header_at (input_location)) >> - pedwarn (input_location, OPT_Wpedantic, "ISO C++ prohibits anonymous structs"); >> + pedwarn (DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (declared_type)), >> + OPT_Wpedantic, "ISO C++ prohibits anonymous structs"); > The change looks fine but the in_system_header_at line can be dropped, right? What do you mean, exactly? Do you mean that, more correctly, we should use DECL_SOURCE_LOCATION for it too or that in fact is and was already completely redundant? I agree with the former, I didn't bother changing it (likely in a couple of other places too) because in practice input_location should work fine anyway as far as noticing that we are in system header is concerned and is simpler. If you mean the latter, I'm not sure, I don't really see why... Paolo.
On Fri, Oct 11, 2019 at 05:14:33PM +0200, Paolo Carlini wrote: > Hi, > > On 11/10/19 15:37, Marek Polacek wrote: > > > > > Index: cp/decl.c > > > =================================================================== > > > --- cp/decl.c (revision 276845) > > > +++ cp/decl.c (working copy) > > > @@ -4992,7 +4992,8 @@ check_tag_decl (cp_decl_specifier_seq *declspecs, > > > if (TREE_CODE (declared_type) != UNION_TYPE > > > && !in_system_header_at (input_location)) > > > - pedwarn (input_location, OPT_Wpedantic, "ISO C++ prohibits anonymous structs"); > > > + pedwarn (DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (declared_type)), > > > + OPT_Wpedantic, "ISO C++ prohibits anonymous structs"); > > The change looks fine but the in_system_header_at line can be dropped, right? > > What do you mean, exactly? Do you mean that, more correctly, we should use > DECL_SOURCE_LOCATION for it too or that in fact is and was already > completely redundant? I agree with the former, I didn't bother changing it > (likely in a couple of other places too) because in practice input_location > should work fine anyway as far as noticing that we are in system header is > concerned and is simpler. If you mean the latter, I'm not sure, I don't > really see why... I mean the latter; pedwarn will check for diagnostic_report_warnings_p so the pedwarn will not trigger in a system header unless -Wsystem-headers even without that check. I know you're just changing the location so you don't need to drop it. I wouldn't worry about the former, I guess it would only make a difference when the { comes from a system header and the } doesn't. -- Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
Hi, On 11/10/19 17:23, Marek Polacek wrote: > I mean the latter; pedwarn will check for diagnostic_report_warnings_p so > the pedwarn will not trigger in a system header unless -Wsystem-headers > even without that check. Oh nice, I wasn't aware of that, to be honest, probably we should audit the front-end for more such redundant uses. Anyway, consider it dropped in this case, I'm re-running the testsuite to be safe. Thanks, Paolo.
Hi again... On 11/10/19 17:30, Paolo Carlini wrote: > Oh nice, I wasn't aware of that, to be honest, probably we should > audit the front-end for more such redundant uses. ... and I can confirm that we have *many*. If we agree that removing all of them is the way to go I can do that in a follow up patch. Thanks, Paolo.
On Fri, Oct 11, 2019 at 05:34:16PM +0200, Paolo Carlini wrote: > Hi again... > > On 11/10/19 17:30, Paolo Carlini wrote: > > Oh nice, I wasn't aware of that, to be honest, probably we should audit > > the front-end for more such redundant uses. > > ... and I can confirm that we have *many*. If we agree that removing all of > them is the way to go I can do that in a follow up patch. If they just guard a pedwarn/warning/warning_at etc., that's fine, if they guard also some code that needs to compute something for the diagnostics, it might not be redundant. Jakub
Hi, On 11/10/19 17:52, Jakub Jelinek wrote: > On Fri, Oct 11, 2019 at 05:34:16PM +0200, Paolo Carlini wrote: >> Hi again... >> >> On 11/10/19 17:30, Paolo Carlini wrote: >>> Oh nice, I wasn't aware of that, to be honest, probably we should audit >>> the front-end for more such redundant uses. >> ... and I can confirm that we have *many*. If we agree that removing all of >> them is the way to go I can do that in a follow up patch. > If they just guard a pedwarn/warning/warning_at etc., that's fine, if they > guard also some code that needs to compute something for the diagnostics, > it might not be redundant. Indeed. We got many of the very straightforward ones ;) Paolo.
On Fri, Oct 11, 2019 at 05:54:43PM +0200, Paolo Carlini wrote: > Hi, > > On 11/10/19 17:52, Jakub Jelinek wrote: > > On Fri, Oct 11, 2019 at 05:34:16PM +0200, Paolo Carlini wrote: > > > Hi again... > > > > > > On 11/10/19 17:30, Paolo Carlini wrote: > > > > Oh nice, I wasn't aware of that, to be honest, probably we should audit > > > > the front-end for more such redundant uses. > > > ... and I can confirm that we have *many*. If we agree that removing all of > > > them is the way to go I can do that in a follow up patch. > > If they just guard a pedwarn/warning/warning_at etc., that's fine, if they > > guard also some code that needs to compute something for the diagnostics, > > it might not be redundant. Yeah, much like with warn_foo guards. > Indeed. We got many of the very straightforward ones ;) Sounds like a nice cleanup! -- Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
On 10/11/19 6:58 AM, Paolo Carlini wrote: > Hi, > > another straightforward DECL_SOURCE_LOCATION (TYPE_MAIN_DECL consistent > with the one I used in build_anon_union_vars. Tested x86_64-linux. > > Thanks, Paolo. > > ///////////////////// OK. Jason
Index: cp/decl.c =================================================================== --- cp/decl.c (revision 276845) +++ cp/decl.c (working copy) @@ -4992,7 +4992,8 @@ check_tag_decl (cp_decl_specifier_seq *declspecs, if (TREE_CODE (declared_type) != UNION_TYPE && !in_system_header_at (input_location)) - pedwarn (input_location, OPT_Wpedantic, "ISO C++ prohibits anonymous structs"); + pedwarn (DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (declared_type)), + OPT_Wpedantic, "ISO C++ prohibits anonymous structs"); } else Index: testsuite/g++.dg/cpp0x/constexpr-union5.C =================================================================== --- testsuite/g++.dg/cpp0x/constexpr-union5.C (revision 276845) +++ testsuite/g++.dg/cpp0x/constexpr-union5.C (working copy) @@ -23,16 +23,16 @@ SA((a.i == 42)); struct B { - struct { + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct" } int h; - struct { + struct { // { dg-warning "12:ISO C\\+\\+ prohibits anonymous struct" } union { unsigned char i; int j; }; int k; - }; // { dg-warning "anonymous struct" } - }; // { dg-warning "anonymous struct" } + }; + }; int l; constexpr B(): h(1), i(2), k(3), l(4) {} Index: testsuite/g++.dg/diagnostic/bitfld2.C =================================================================== --- testsuite/g++.dg/diagnostic/bitfld2.C (revision 276845) +++ testsuite/g++.dg/diagnostic/bitfld2.C (working copy) @@ -6,4 +6,4 @@ template<int> struct A struct {} : 2; // { dg-error "expected ';' after struct" "expected" } }; // { dg-error "ISO C.. forbids declaration" "declaration" { target *-*-* } 6 } -// { dg-error "ISO C.. prohibits anonymous" "anonymous" { target *-*-* } 6 } +// { dg-error "10:ISO C.. prohibits anonymous" "anonymous" { target *-*-* } 6 } Index: testsuite/g++.dg/ext/anon-struct1.C =================================================================== --- testsuite/g++.dg/ext/anon-struct1.C (revision 276845) +++ testsuite/g++.dg/ext/anon-struct1.C (working copy) @@ -19,7 +19,7 @@ char testD[sizeof(C::D) == sizeof(A) ? 1 : -1]; /* GNU extension. */ struct E { - struct { char z; }; /* { dg-error "prohibits anonymous structs" } */ + struct { char z; }; /* { dg-error "10:ISO C\\+\\+ prohibits anonymous structs" } */ char e; }; @@ -45,6 +45,6 @@ char testH[sizeof(H) == 2 * sizeof(A) ? 1 : -1]; /* Make sure __extension__ gets turned back off. */ struct I { - struct { char z; }; /* { dg-error "prohibits anonymous structs" } */ + struct { char z; }; /* { dg-error "10:ISO C\\+\\+ prohibits anonymous structs" } */ char i; }; Index: testsuite/g++.dg/ext/anon-struct6.C =================================================================== --- testsuite/g++.dg/ext/anon-struct6.C (revision 276845) +++ testsuite/g++.dg/ext/anon-struct6.C (working copy) @@ -3,8 +3,8 @@ struct A { struct - { + { // { dg-error "3:ISO C\\+\\+ prohibits anonymous structs" } struct { static int i; }; // { dg-error "prohibits anonymous structs|non-static data members|unnamed class" } void foo() { i; } // { dg-error "public non-static data" } - }; // { dg-error "prohibits anonymous structs" } + }; }; Index: testsuite/g++.dg/ext/flexary19.C =================================================================== --- testsuite/g++.dg/ext/flexary19.C (revision 276845) +++ testsuite/g++.dg/ext/flexary19.C (working copy) @@ -146,13 +146,13 @@ struct S16 { int i; - struct { // { dg-warning "invalid use" } + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct|invalid use" } // A flexible array as a sole member of an anonymous struct is // rejected with an error in C mode but emits just a pedantic // warning in C++. Other than excessive pedantry there is no // reason to reject it. int a[]; - }; // { dg-warning "anonymous struct" } + }; }; struct S17 @@ -177,9 +177,9 @@ struct S19 { int i; - struct { // { dg-warning "invalid use" } + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct|invalid use" } int j, a[]; // { dg-message "declared here" } - }; // { dg-warning "anonymous struct" } + }; }; struct S20 @@ -198,10 +198,10 @@ struct S21 static int i; typedef int A[]; - struct { // { dg-warning "invalid use" } + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct|invalid use" } int j; A a; // { dg-message "declared here" } - }; // { dg-warning "anonymous struct" } + }; }; struct S22 @@ -215,11 +215,11 @@ struct S22 struct S23 { - struct { + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct" } static int i; // { dg-error "static data member" } int a[]; // { dg-error "in an otherwise empty" } - }; // { dg-warning "anonymous struct" } + }; }; struct S24 @@ -296,11 +296,11 @@ union A union B { - struct { - struct { // { dg-warning "invalid use" } + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct" } + struct { // { dg-warning "12:ISO C\\+\\+ prohibits anonymous struct|invalid use" } int i, a[]; // { dg-message "declared here" } - }; // { dg-warning "anonymous struct" } - }; // { dg-warning "anonymous struct" } + }; + }; int j; }; Index: testsuite/g++.dg/ext/flexary9.C =================================================================== --- testsuite/g++.dg/ext/flexary9.C (revision 276845) +++ testsuite/g++.dg/ext/flexary9.C (working copy) @@ -282,64 +282,64 @@ struct S_S_S_x { struct Anon1 { int n; - struct { // { dg-warning "invalid use \[^\n\r\]* with a zero-size array" } + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct|invalid use \[^\n\r\]* with a zero-size array" } int good[0]; // { dg-warning "forbids zero-size array" } - }; // { dg-warning "anonymous struct" } + }; }; ASSERT_AT_END (Anon1, good); struct Anon2 { - struct { // { dg-warning "invalid use" } + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct|invalid use" } int n; - struct { + struct { // { dg-warning "12:ISO C\\+\\+ prohibits anonymous struct" } int good[0]; // { dg-warning "zero-size array" } - }; // { dg-warning "anonymous struct" } - }; // { dg-warning "anonymous struct" } + }; + }; }; ASSERT_AT_END (Anon2, good); struct Anon3 { - struct { // { dg-warning "invalid use" } - struct { + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct|invalid use" } + struct { // { dg-warning "12:ISO C\\+\\+ prohibits anonymous struct" } int n; int good[0]; // { dg-warning "zero-size array" } - }; // { dg-warning "anonymous struct" } - }; // { dg-warning "anonymous struct" } + }; + }; }; ASSERT_AT_END (Anon3, good); struct Anon4 { - struct { + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct" } int in_empty_struct[0]; // { dg-warning "zero-size array|in an otherwise empty" } - }; // { dg-warning "anonymous struct" } + }; }; struct Anon5 { - struct { + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct" } int not_at_end[0]; // { dg-warning "zero-size array|not at end" } - }; // { dg-warning "anonymous struct" } + }; int n; }; struct Anon6 { - struct { - struct { + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct" } + struct { // { dg-warning "12:ISO C\\+\\+ prohibits anonymous struct" } int not_at_end[0]; // { dg-warning "zero-size array|not at end" } - }; // { dg-warning "anonymous struct" } + }; int n; - }; // { dg-warning "anonymous struct" } + }; }; struct Anon7 { - struct { - struct { + struct { // { dg-warning "10:ISO C\\+\\+ prohibits anonymous struct" } + struct { // { dg-warning "12:ISO C\\+\\+ prohibits anonymous struct" } int not_at_end[0]; // { dg-warning "zero-size array|not at end" } - }; // { dg-warning "anonymous struct" } - }; // { dg-warning "anonymous struct" } + }; + }; int n; }; Index: testsuite/g++.dg/template/error17.C =================================================================== --- testsuite/g++.dg/template/error17.C (revision 276845) +++ testsuite/g++.dg/template/error17.C (working copy) @@ -4,6 +4,6 @@ template <typename T> void foo() { - union { struct { }; }; // { dg-error "prohibits anonymous struct" "anon" } + union { struct { }; }; // { dg-error "18:ISO C\\+\\+ prohibits anonymous struct" } // { dg-error "18:anonymous struct not inside" "not inside" { target *-*-* } .-1 } }