[PATCH 33/348] Fix -Wsahdow warnings

[PATCH 33/348] Fix -Wsahdow warnings

[Andrey Smirnov]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Fix -Wshadow warnings --]
[-- Type: text/x-diff, Size: 2965 bytes --]

From 9b61ead63ef1410792628c11b8ffe8f5d914a67f Mon Sep 17 00:00:00 2001
From: Andrey Smirnov <andrew.smirnov@gmail.com>
Date: Tue, 22 Nov 2011 17:55:58 +0700
Subject: [PATCH 33/39] Fix -Wshadow warnings.

* breakpoint.c (update_global_location_list): Fix -Wshadow
warnings.
---
 gdb/ChangeLog    |    5 +++++
 gdb/breakpoint.c |   22 +++++++++++-----------
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d82ee5c..77bff37 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
 
+	* breakpoint.c (update_global_location_list): Fix -Wshadow
+	warnings.
+
+2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
+
 	* breakpoint.c (watch_command_1): Fix -Wshadow
 	warnings.
 
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1a4974c..cba4d3e 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10667,32 +10667,32 @@ update_global_location_list (int should_insert)
     {
       /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always
 	 non-NULL.  */
-      struct breakpoint *b = loc->owner;
+      struct breakpoint *bp = loc->owner;
       struct bp_location **loc_first_p;
 
-      if (b->enable_state == bp_disabled
-	  || b->enable_state == bp_call_disabled
-	  || b->enable_state == bp_startup_disabled
+      if (bp->enable_state == bp_disabled
+	  || bp->enable_state == bp_call_disabled
+	  || bp->enable_state == bp_startup_disabled
 	  || !loc->enabled
 	  || loc->shlib_disabled
-	  || !breakpoint_address_is_meaningful (b)
+	  || !breakpoint_address_is_meaningful (bp)
 	  /* Don't detect duplicate for tracepoint locations because they are
 	   never duplicated.  See the comments in field `duplicate' of
 	   `struct bp_location'.  */
-	  || is_tracepoint (b))
+	  || is_tracepoint (bp))
 	continue;
 
       /* Permanent breakpoint should always be inserted.  */
-      if (b->enable_state == bp_permanent && ! loc->inserted)
+      if (bp->enable_state == bp_permanent && ! loc->inserted)
 	internal_error (__FILE__, __LINE__,
 			_("allegedly permanent breakpoint is not "
 			"actually inserted"));
 
-      if (b->type == bp_hardware_watchpoint)
+      if (bp->type == bp_hardware_watchpoint)
 	loc_first_p = &wp_loc_first;
-      else if (b->type == bp_read_watchpoint)
+      else if (bp->type == bp_read_watchpoint)
 	loc_first_p = &rwp_loc_first;
-      else if (b->type == bp_access_watchpoint)
+      else if (bp->type == bp_access_watchpoint)
 	loc_first_p = &awp_loc_first;
       else
 	loc_first_p = &bp_loc_first;
@@ -10715,7 +10715,7 @@ update_global_location_list (int should_insert)
       loc->duplicate = 1;
 
       if ((*loc_first_p)->owner->enable_state == bp_permanent && loc->inserted
-	  && b->enable_state != bp_permanent)
+	  && bp->enable_state != bp_permanent)
 	internal_error (__FILE__, __LINE__,
 			_("another breakpoint was inserted on top of "
 			"a permanent breakpoint"));
-- 
1.7.5.4

Re: [PATCH 33/348] Fix -Wsahdow warnings

[Tom Tromey]

>>>>> "Andrey" == Andrey Smirnov <andrew.smirnov@gmail.com> writes:

Andrey> * breakpoint.c (update_global_location_list): Fix -Wshadow
Andrey> warnings.

Andrey> -      struct breakpoint *b = loc->owner;
Andrey> +      struct breakpoint *bp = loc->owner;

I think in this case it is ok to just reuse the enclosing 'b'.
That is, just delete this declaration.
'b' is just used as an iterator in this function.

Tom

[PATCH 033/238] [misc.] breakpoint.c: -Wshadow fix

[Andrey Smirnov]

Cause:
       Variable in the inner scope shadowed by variable from outer
       one(defined at the beginning of function.

To ChangeLog:
	* breakpoint.c (update_global_location_list): Remove nested
        declaration of `b'(-Wshadow).
---
 gdb/ChangeLog    |    5 +++++
 gdb/breakpoint.c |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index cf6cc8f..f433ef5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
 
+	* breakpoint.c (update_global_location_list): Fix -Wshadow
+	warnings.
+
+2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
+
 	* breakpoint.c (watch_command_1): Fix -Wshadow
 	warnings.
 
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1a4974c..69a8782 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10667,8 +10667,8 @@ update_global_location_list (int should_insert)
     {
       /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always
 	 non-NULL.  */
-      struct breakpoint *b = loc->owner;
       struct bp_location **loc_first_p;
+      b = loc->owner;
 
       if (b->enable_state == bp_disabled
 	  || b->enable_state == bp_call_disabled
-- 
1.7.5.4

Re: [PATCH 033/238] [misc.] breakpoint.c: -Wshadow fix

[Joel Brobecker]

> Cause:
>        Variable in the inner scope shadowed by variable from outer
>        one(defined at the beginning of function.
[...]
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 1a4974c..69a8782 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -10667,8 +10667,8 @@ update_global_location_list (int should_insert)
>      {
>        /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always
>  	 non-NULL.  */
> -      struct breakpoint *b = loc->owner;
>        struct bp_location **loc_first_p;
> +      b = loc->owner;
>  
>        if (b->enable_state == bp_disabled
>  	  || b->enable_state == bp_call_disabled

I looked at it, and it looks fine from a functional point of view.
However, I'd rather have Jan or Pedro, who have modified this function
more often than I have, to weigh in.

Personally, I'm not keen on the fact that a global variable is reused
in the context of a local loop. So I would rather rename the local
variable inside the loop rather than delete the local variable,
and reuse the global one.  It makes for a bigger patch, but I think
it's better in the end.

-- 
Joel

Re: [PATCH 033/238] [misc.] breakpoint.c: -Wshadow fix

[Andrey Smirnov]

> I looked at it, and it looks fine from a functional point of view.
> However, I'd rather have Jan or Pedro, who have modified this function
> more often than I have, to weigh in.
>
> Personally, I'm not keen on the fact that a global variable is reused
> in the context of a local loop. So I would rather rename the local
> variable inside the loop rather than delete the local variable,
> and reuse the global one.  It makes for a bigger patch, but I think
> it's better in the end.

I tried to used the strategy you describe in all my patches and initial
patch actually renamed the local variable, this modification was made
because of Tom's recommendation, see
http://sourceware.org/ml/gdb-patches/2011-11/msg00601.html

So, I guess we will have to get to some sort of consensus on how to deal
with that name clash.

Andrey Smirnov

Re: [PATCH 033/238] [misc.] breakpoint.c: -Wshadow fix

[Pedro Alves]

On Monday 05 December 2011 11:25:23, Joel Brobecker wrote:
> > Cause:
> >        Variable in the inner scope shadowed by variable from outer
> >        one(defined at the beginning of function.
> [...]
> > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> > index 1a4974c..69a8782 100644
> > --- a/gdb/breakpoint.c
> > +++ b/gdb/breakpoint.c
> > @@ -10667,8 +10667,8 @@ update_global_location_list (int should_insert)
> >      {
> >        /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always
> >  	 non-NULL.  */
> > -      struct breakpoint *b = loc->owner;
> >        struct bp_location **loc_first_p;
> > +      b = loc->owner;
> >  
> >        if (b->enable_state == bp_disabled
> >  	  || b->enable_state == bp_call_disabled
> 
> I looked at it, and it looks fine from a functional point of view.
> However, I'd rather have Jan or Pedro, who have modified this function
> more often than I have, to weigh in.

Looks fine to me.

> Personally, I'm not keen on the fact that a global variable is reused
> in the context of a local loop. So I would rather rename the local
> variable inside the loop rather than delete the local variable,
> and reuse the global one.  It makes for a bigger patch, but I think
> it's better in the end.

I'd agree if e.g., `b' was a passed in function argument, that had
some special meaning to the function.  But `b' in this function
is really just a temporary variable helper for the
ALL_BREAKPOINTS loops.

-- 
Pedro Alves