diff mbox series

[C++] One more DECL_SOURCE_LOCATION

Message ID ca1511be-8e53-05a1-c3e2-fcf40f7e8329@oracle.com
State New
Headers show
Series [C++] One more DECL_SOURCE_LOCATION | expand

Commit Message

Paolo Carlini Oct. 11, 2019, 10:58 a.m. UTC
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.

Comments

Marek Polacek Oct. 11, 2019, 1:37 p.m. UTC | #1
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
Paolo Carlini Oct. 11, 2019, 3:14 p.m. UTC | #2
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.
Marek Polacek Oct. 11, 2019, 3:23 p.m. UTC | #3
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
Paolo Carlini Oct. 11, 2019, 3:30 p.m. UTC | #4
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.
Paolo Carlini Oct. 11, 2019, 3:34 p.m. UTC | #5
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.
Jakub Jelinek Oct. 11, 2019, 3:52 p.m. UTC | #6
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
Paolo Carlini Oct. 11, 2019, 3:54 p.m. UTC | #7
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.
Marek Polacek Oct. 11, 2019, 3:58 p.m. UTC | #8
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
Jason Merrill Oct. 11, 2019, 9:58 p.m. UTC | #9
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
diff mbox series

Patch

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