Re: [C++ Patch] One more DECL_SOURCE_LOCATION

[Jason Merrill]

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

Re: [C++ Patch] One more DECL_SOURCE_LOCATION

[Marek Polacek]

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

Re: [C++ Patch] One more DECL_SOURCE_LOCATION

[Paolo Carlini]

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.

Re: [C++ Patch] One more DECL_SOURCE_LOCATION

[Jakub Jelinek]

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

Re: [C++ Patch] One more DECL_SOURCE_LOCATION

[Paolo Carlini]

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.

Re: [C++ Patch] One more DECL_SOURCE_LOCATION

[Paolo Carlini]

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.

Re: [C++ Patch] One more DECL_SOURCE_LOCATION

[Marek Polacek]

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

Re: [C++ Patch] One more DECL_SOURCE_LOCATION

[Paolo Carlini]

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.

Re: [C++ Patch] One more DECL_SOURCE_LOCATION

[Marek Polacek]

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  
> 
>   * decl.c (check_tag_decl): Use DECL_SOURCE_LOCATION.
> 
> /testsuite
> 2019-10-11  Paolo Carlini  
> 
>   * 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

[C++ Patch] One more DECL_SOURCE_LOCATION

[Paolo Carlini]

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  

* decl.c (check_tag_decl): Use DECL_SOURCE_LOCATION.

/testsuite
2019-10-11  Paolo Carlini  

* 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");
 }
 
   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 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|in