[PATCH 22/348] Fix -Wsahdow warnings

[PATCH 22/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: 3232 bytes --]

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

* bcache.c (expand_hash_table): Fix -Wshadow
warnings.
---
 gdb/ChangeLog |    5 +++++
 gdb/bcache.c  |   30 +++++++++++++++---------------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f6711e4..d972e6a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
 
+	* bcache.c (expand_hash_table): Fix -Wshadow
+	warnings.
+
+2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
+
 	* annotate.c (annotate_array_section_begin): Fix -Wshadow
 	warnings.
 
diff --git a/gdb/bcache.c b/gdb/bcache.c
index 76e3893..f7543f4 100644
--- a/gdb/bcache.c
+++ b/gdb/bcache.c
@@ -130,7 +130,7 @@ hash_continue (const void *addr, int length, unsigned long h)
 #define CHAIN_LENGTH_THRESHOLD (5)
 
 static void
-expand_hash_table (struct bcache *bcache)
+expand_hash_table (struct bcache *cahce)
 {
   /* A table of good hash table sizes.  Whenever we grow, we pick the
      next larger size from this table.  sizes[i] is close to 1 << (i+10),
@@ -149,13 +149,13 @@ expand_hash_table (struct bcache *bcache)
 
   /* Count the stats.  Every unique item needs to be re-hashed and
      re-entered.  */
-  bcache->expand_count++;
-  bcache->expand_hash_count += bcache->unique_count;
+  cahce->expand_count++;
+  cahce->expand_hash_count += cahce->unique_count;
 
   /* Find the next size.  */
-  new_num_buckets = bcache->num_buckets * 2;
+  new_num_buckets = cahce->num_buckets * 2;
   for (i = 0; i < (sizeof (sizes) / sizeof (sizes[0])); i++)
-    if (sizes[i] > bcache->num_buckets)
+    if (sizes[i] > cahce->num_buckets)
       {
 	new_num_buckets = sizes[i];
 	break;
@@ -168,22 +168,22 @@ expand_hash_table (struct bcache *bcache)
     new_buckets = (struct bstring **) xmalloc (new_size);
     memset (new_buckets, 0, new_size);
 
-    bcache->structure_size -= (bcache->num_buckets
-			       * sizeof (bcache->bucket[0]));
-    bcache->structure_size += new_size;
+    cahce->structure_size -= (cahce->num_buckets
+			       * sizeof (cahce->bucket[0]));
+    cahce->structure_size += new_size;
   }
 
   /* Rehash all existing strings.  */
-  for (i = 0; i < bcache->num_buckets; i++)
+  for (i = 0; i < cahce->num_buckets; i++)
     {
       struct bstring *s, *next;
 
-      for (s = bcache->bucket[i]; s; s = next)
+      for (s = cahce->bucket[i]; s; s = next)
 	{
 	  struct bstring **new_bucket;
 	  next = s->next;
 
-	  new_bucket = &new_buckets[(bcache->hash_function (&s->d.data,
+	  new_bucket = &new_buckets[(cahce->hash_function (&s->d.data,
 							    s->length)
 				     % new_num_buckets)];
 	  s->next = *new_bucket;
@@ -192,10 +192,10 @@ expand_hash_table (struct bcache *bcache)
     }
 
   /* Plug in the new table.  */
-  if (bcache->bucket)
-    xfree (bcache->bucket);
-  bcache->bucket = new_buckets;
-  bcache->num_buckets = new_num_buckets;
+  if (cahce->bucket)
+    xfree (cahce->bucket);
+  cahce->bucket = new_buckets;
+  cahce->num_buckets = new_num_buckets;
 }
 
 \f
-- 
1.7.5.4

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

[Eli Zaretskii]

> From: Andrey Smirnov <andrew.smirnov@gmail.com>
> Date: Tue, 22 Nov 2011 20:07:24 +0700
> 
> -expand_hash_table (struct bcache *bcache)
> +expand_hash_table (struct bcache *cahce)
                                     ^^^^^
Is the typo intentional?

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

[Andrey Smirnov]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrey Smirnov <andrew.smirnov@gmail.com>
>> Date: Tue, 22 Nov 2011 20:07:24 +0700
>> 
>> -expand_hash_table (struct bcache *bcache)
>> +expand_hash_table (struct bcache *cahce)
>                                      ^^^^^
> Is the typo intentional?

Sorry about that. No it isn't. Please ignore that particular patch I'll
send corrected version soon(I'm in the middle of a git rebase
--interactive, so I can't edit that particular commit just yet).

Andrey Smirnov

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

[Marek Polacek]

On 11/22/2011 02:33 PM, Andrey Smirnov wrote:
>> Is the typo intentional?
> 
> Sorry about that. No it isn't. Please ignore that particular patch I'll
> send corrected version soon(I'm in the middle of a git rebase
> --interactive, so I can't edit that particular commit just yet).

That typo is in other patches too.

	Marek

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

[Andrey Smirnov]

On Tue, Nov 22, 2011 at 8:35 PM, Marek Polacek <mpolacek@redhat.com> wrote:
> On 11/22/2011 02:33 PM, Andrey Smirnov wrote:
>>> Is the typo intentional?
>>
>> Sorry about that. No it isn't. Please ignore that particular patch I'll
>> send corrected version soon(I'm in the middle of a git rebase
>> --interactive, so I can't edit that particular commit just yet).
>
> That typo is in other patches too.
>
>        Marek
>

OK, once again, sorry about that, I'll recheck all the patches related
to bcache.c, for now please ignore those.

Andrey Smirnov

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

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 957 bytes --]

On Tuesday 22 November 2011 09:04:07 Andrey Smirnov wrote:
> On Tue, Nov 22, 2011 at 8:35 PM, Marek Polacek <mpolacek@redhat.com> wrote:
> > On 11/22/2011 02:33 PM, Andrey Smirnov wrote:
> >>> Is the typo intentional?
> >> 
> >> Sorry about that. No it isn't. Please ignore that particular patch I'll
> >> send corrected version soon(I'm in the middle of a git rebase
> >> --interactive, so I can't edit that particular commit just yet).
> > 
> > That typo is in other patches too.
> 
> OK, once again, sorry about that, I'll recheck all the patches related
> to bcache.c, for now please ignore those.

please condense down your patches if you resend.  there's way too many little 
tiny ones that really should be squashed into a single changeset.

your ChangeLogs are also incorrect.  it should not be:
	* bcache.c (expand_hash_table): Fix -Wshadow warnings.
but rather:
	* bcache.c (expand_hash_table): Rename bcache to cache.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Joel Brobecker]

> please condense down your patches if you resend.  there's way too many
> little tiny ones that really should be squashed into a single
> changeset.

In my view, if the patches can be checked in independently, then
it is a good thing that they are split. Imagine the situation where
one of these changes is bad, we'd then be able to revert that one
patch, rather than fixing by hand.

> your ChangeLogs are also incorrect.  it should not be:
> 	* bcache.c (expand_hash_table): Fix -Wshadow warnings.
> but rather:
> 	* bcache.c (expand_hash_table): Rename bcache to cache.

I'm 50/50 on this. I don't mind either way. What do others think?
Is that really that important that we must create boring extra work
for Andrey?

-- 
Joel

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

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1153 bytes --]

On Tuesday 22 November 2011 11:05:39 Joel Brobecker wrote:
> > please condense down your patches if you resend.  there's way too many
> > little tiny ones that really should be squashed into a single
> > changeset.
> 
> In my view, if the patches can be checked in independently, then
> it is a good thing that they are split. Imagine the situation where
> one of these changes is bad, we'd then be able to revert that one
> patch, rather than fixing by hand.

i'm not saying it should be exactly 1 patch.  but 348 is way too big.  doing 
it on an API or file level is a good compromise.

> > your ChangeLogs are also incorrect.  it should not be:
> > 	* bcache.c (expand_hash_table): Fix -Wshadow warnings.
> > 
> > but rather:
> > 	* bcache.c (expand_hash_table): Rename bcache to cache.
> 
> I'm 50/50 on this. I don't mind either way. What do others think?
> Is that really that important that we must create boring extra work
> for Andrey?

it's my understanding that the GNU changelog style is "document what changed" 
and not "why".  i think that's largely stupid, but i'm not the one in control 
of said policy.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Doug Evans]

On Tue, Nov 22, 2011 at 8:20 AM, Mike Frysinger <vapier@gentoo.org> wrote:
>> > your ChangeLogs are also incorrect.  it should not be:
>> >     * bcache.c (expand_hash_table): Fix -Wshadow warnings.
>> >
>> > but rather:
>> >     * bcache.c (expand_hash_table): Rename bcache to cache.
>>
>> I'm 50/50 on this. I don't mind either way. What do others think?
>> Is that really that important that we must create boring extra work
>> for Andrey?
>
> it's my understanding that the GNU changelog style is "document what changed"
> and not "why".  i think that's largely stupid, but i'm not the one in control
> of said policy.

For little things like this, at least for those that are internal to a function,
I've seen a lot of leeway and I think that's ok.
fwiw.

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

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> please condense down your patches if you resend.  there's way too many
>> little tiny ones that really should be squashed into a single
>> changeset.

Joel> In my view, if the patches can be checked in independently, then
Joel> it is a good thing that they are split. Imagine the situation where
Joel> one of these changes is bad, we'd then be able to revert that one
Joel> patch, rather than fixing by hand.

I think a bit more batching would be ok.
It doesn't really matter to me, though, I will go through them all
either way.

>> your ChangeLogs are also incorrect.  it should not be:
>> * bcache.c (expand_hash_table): Fix -Wshadow warnings.
>> but rather:
>> * bcache.c (expand_hash_table): Rename bcache to cache.

Joel> I'm 50/50 on this. I don't mind either way. What do others think?
Joel> Is that really that important that we must create boring extra work
Joel> for Andrey?

I can't imagine rewriting 348 ChangeLog entries.

Tom

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

[Mark Kettenis]

> Date: Tue, 22 Nov 2011 08:05:39 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > please condense down your patches if you resend.  there's way too many
> > little tiny ones that really should be squashed into a single
> > changeset.
> 
> In my view, if the patches can be checked in independently, then
> it is a good thing that they are split. Imagine the situation where
> one of these changes is bad, we'd then be able to revert that one
> patch, rather than fixing by hand.

But sending 388 seperate changes is insane.

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

[Andrey Smirnov]

On Wed, Nov 23, 2011 at 11:45 PM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> Date: Tue, 22 Nov 2011 08:05:39 -0800
>> From: Joel Brobecker <brobecker@adacore.com>
>>
> > please condense down your patches if you resend.  there's way too many
> > little tiny ones that really should be squashed into a single
>> > changeset.
>>
>> In my view, if the patches can be checked in independently, then
>> it is a good thing that they are split. Imagine the situation where
>> one of these changes is bad, we'd then be able to revert that one
>> patch, rather than fixing by hand.
>
> But sending 388 seperate changes is insane.

OK, what is the maximum amount of patches that one is allowed to have
in a patchset?

Andrey Smirnov

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

[Andrey Smirnov]

On Tue, Nov 22, 2011 at 10:27 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tuesday 22 November 2011 09:04:07 Andrey Smirnov wrote:
>> On Tue, Nov 22, 2011 at 8:35 PM, Marek Polacek <mpolacek@redhat.com> wrote:
>> > On 11/22/2011 02:33 PM, Andrey Smirnov wrote:
>> >>> Is the typo intentional?
>> >>
>> >> Sorry about that. No it isn't. Please ignore that particular patch I'll
>> >> send corrected version soon(I'm in the middle of a git rebase
>> >> --interactive, so I can't edit that particular commit just yet).
>> >
>> > That typo is in other patches too.
>>
>> OK, once again, sorry about that, I'll recheck all the patches related
>> to bcache.c, for now please ignore those.
>
> please condense down your patches if you resend.  there's way too many little
> tiny ones that really should be squashed into a single changeset.
>

Initially, there were 17 patches, which, upon suggestion from Tom
Tromey, I split so that every patch contain only changes to one
particular function or some other small unit of the source code. I
tend to agree with Tom that my initial decision to make only 17
patches made it rather hard to review each, because every one of them
contained many small but disparate changes.

Squashing or splitting commits is not really a problem and I can do
this, but if you want me to do so, than please point out the patches
I should squash together.

Right now I have 258 patches to which I have yet to write a ChangeLog
entry and 100 patches whose ChangeLog I have to change to, as you
pointed out, conform to GNU policy. There are also patches that I
screwed up with typos and patches that will eventually will have to be
rewritten.

Squashing all commits based on file level will still leave you with
something like 150 patches, but some of them would be quite large and
harder to review than they are now. Doing so on API level would
require me to go through all the patches and relative source code,
figure out to what API every change belong(which I'll probably do wrong
because I do not yet have a very good grasp on GDB's internals) and
split and squash all commits accordingly.

So given the aforementioned amount of work, can't we ignore that the
patch count is over 9000?

Andrey Smirnov

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

[Tom Tromey]

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

Andrey> Initially, there were 17 patches, which, upon suggestion from Tom
Andrey> Tromey, I split so that every patch contain only changes to one
Andrey> particular function or some other small unit of the source code. I
Andrey> tend to agree with Tom that my initial decision to make only 17
Andrey> patches made it rather hard to review each, because every one of them
Andrey> contained many small but disparate changes.

I didn't really mean for you to split it down this much, but now you've
done it.  I don't want to make too much extra work for you.

Andrey> Squashing or splitting commits is not really a problem and I can do
Andrey> this, but if you want me to do so, than please point out the patches
Andrey> I should squash together.

It is hard for us to do that without seeing the whole series :)

Conversely maybe it is hard for you to know which patches are likely to
be controversial and which are obvious.

I think it is hard to discuss in the abstract.  One idea would be for
you to merge reasonably obvious patches together in a file-based way,
using your best judgment about what "reasonably obvious" means.

Or, we can just carry on.

Andrey> So given the aforementioned amount of work, can't we ignore that the
Andrey> patch count is over 9000?

Not sure what this refers to.

Tom

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

[Andrey Smirnov]

On Thu, Nov 24, 2011 at 12:06 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Andrey" == Andrey Smirnov <andrew.smirnov@gmail.com> writes:
>
> Andrey> Initially, there were 17 patches, which, upon suggestion from Tom
> Andrey> Tromey, I split so that every patch contain only changes to one
> Andrey> particular function or some other small unit of the source code. I
> Andrey> tend to agree with Tom that my initial decision to make only 17
> Andrey> patches made it rather hard to review each, because every one of them
> Andrey> contained many small but disparate changes.
>
> I didn't really mean for you to split it down this much, but now you've
> done it.  I don't want to make too much extra work for you.

Et tu, Brute? :) I'll try to condense the patches.

> Andrey> Squashing or splitting commits is not really a problem and I can do
> Andrey> this, but if you want me to do so, than please point out the patches
> Andrey> I should squash together.
>
> It is hard for us to do that without seeing the whole series :)

Well, this problem can be solved quite easily. As soon as I'm done
with ChangeLogs I can create a repo on github and all the changes
would be accessible to anyone.

> Conversely maybe it is hard for you to know which patches are likely to
> be controversial and which are obvious.
>
> I think it is hard to discuss in the abstract.  One idea would be for
> you to merge reasonably obvious patches together in a file-based way,
> using your best judgment about what "reasonably obvious" means.
>
> Or, we can just carry on.
>
> Andrey> So given the aforementioned amount of work, can't we ignore that the
> Andrey> patch count is over 9000?
>
> Not sure what this refers to.
>

Internet memes, Dragon Ball Z. Sorry about that. Rephrased the
sentence would be: "So given the aforementioned amount of work, can't
we ignore that the patch count is very large?"

Andrey Smirnov

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

[Doug Evans]

On Tue, Nov 22, 2011 at 9:29 PM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> On Tue, Nov 22, 2011 at 10:27 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>> On Tuesday 22 November 2011 09:04:07 Andrey Smirnov wrote:
>>> On Tue, Nov 22, 2011 at 8:35 PM, Marek Polacek <mpolacek@redhat.com> wrote:
>>> > On 11/22/2011 02:33 PM, Andrey Smirnov wrote:
>>> >>> Is the typo intentional?
>>> >>
>>> >> Sorry about that. No it isn't. Please ignore that particular patch I'll
>>> >> send corrected version soon(I'm in the middle of a git rebase
>>> >> --interactive, so I can't edit that particular commit just yet).
>>> >
>>> > That typo is in other patches too.
>>>
>>> OK, once again, sorry about that, I'll recheck all the patches related
>>> to bcache.c, for now please ignore those.
>>
>> please condense down your patches if you resend.  there's way too many little
>> tiny ones that really should be squashed into a single changeset.
>>
>
> Initially, there were 17 patches, which, upon suggestion from Tom
> Tromey, I split so that every patch contain only changes to one
> particular function or some other small unit of the source code. I
> tend to agree with Tom that my initial decision to make only 17
> patches made it rather hard to review each, because every one of them
> contained many small but disparate changes.
>
> Squashing or splitting commits is not really a problem and I can do
> this, but if you want me to do so, than please point out the patches
> I should squash together.
>
> Right now I have 258 patches to which I have yet to write a ChangeLog
> entry and 100 patches whose ChangeLog I have to change to, as you
> pointed out, conform to GNU policy. There are also patches that I
> screwed up with typos and patches that will eventually will have to be
> rewritten.
>
> Squashing all commits based on file level will still leave you with
> something like 150 patches, but some of them would be quite large and
> harder to review than they are now. Doing so on API level would
> require me to go through all the patches and relative source code,
> figure out to what API every change belong(which I'll probably do wrong
> because I do not yet have a very good grasp on GDB's internals) and
> split and squash all commits accordingly.
>
> So given the aforementioned amount of work, can't we ignore that the
> patch count is over 9000?
>
> Andrey Smirnov
>

For reference sake, I did "grep -e -Wall ChangeLog*" to see what's
been done in the past.  Based on that there is room for compromise I think.

Since these are just mechanical changes, and there are a lot of them,
I'd be happy with a compromise everyone is happy (or at least
not unhappy :-)) with.

I think keeping them at the file level is easiest for you (just
guessing though).
And I'd be happy with a changelog entry that simply said:

        * foo.c: -Wshadow lint.

or

        * foo.c (bar, baz): -Wshadow lint.
        (huey,dewey,louie): Ditto.

These are all just mechanical changes.

btw, I can imagine things regressing if we don't add -Wshadow
to configure.ac:build_warnings (say after the tree is clean).
We're agreed we want that right?  Otherwise ...

Also, given the quantity, I'd suggest holding off until after 7.4 is branched.
[just a suggestion though]

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

[Doug Evans]

On Wed, Nov 23, 2011 at 9:56 AM, Doug Evans <dje@google.com> wrote:
> Also, given the quantity, I'd suggest holding off until after 7.4 is branched.
> [just a suggestion though]

Heh, OTOH ...
It will make backporting fixes into the 7.4 branch harder if a lot go
in right after it's branched.
That suggests holding off until 7.4 is close to going out.

OTOOH, holding off too long will just make it harder for you to keep
your patches up to date.
But I wouldn't hold up 7.4 for these changes.

In the end I think the high order bit is not making it too difficult
to backport fixes into the 7.4 branch.

My $0.02.  Blech.

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

[Joel Brobecker]

> > Also, given the quantity, I'd suggest holding off until after 7.4 is branched.
> > [just a suggestion though]
> 
> Heh, OTOH ...
> It will make backporting fixes into the 7.4 branch harder if a lot go
> in right after it's branched.
> That suggests holding off until 7.4 is close to going out.
> 
> OTOOH, holding off too long will just make it harder for you to keep
> your patches up to date.
> But I wouldn't hold up 7.4 for these changes.
> 
> In the end I think the high order bit is not making it too difficult
> to backport fixes into the 7.4 branch.

I agree that the priority is to make merging easy.

I'm also starting to question the benefits vs cost of enabling
-Wshadow.  In particular, warnings that for us to change the name
of function parameters such as "block_found" or "index" make me feel
like this is going too far.

I think it's useful to review the warnings, because it did find some
real situation where we were shadowing another variable from an outer
scope. But I tend to disagree with a good portion of the warnings
I am seeing right now. Add the fact that the warnings also depend
on the host's system includes, and you might be as uncomfortable
as I am...

-- 
Joel

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

[Andrey Smirnov]

On Thu, Nov 24, 2011 at 12:56 AM, Doug Evans <dje@google.com> wrote:

> For reference sake, I did "grep -e -Wall ChangeLog*" to see what's
> been done in the past.  Based on that there is room for compromise I think.
>
> Since these are just mechanical changes, and there are a lot of them,
> I'd be happy with a compromise everyone is happy (or at least
> not unhappy :-)) with.
>
> I think keeping them at the file level is easiest for you (just
> guessing though).
> And I'd be happy with a changelog entry that simply said:
>
>        * foo.c: -Wshadow lint.
>
> or
>
>        * foo.c (bar, baz): -Wshadow lint.
>        (huey,dewey,louie): Ditto.
>

I changed(after it was pointed out that they do not conform to GNU
policy) the style of my ChangeLog entries to

        * 42.c (trillian): Rename zaphod to beeblebrox(-Wshadow).

Hope this is a reasonable compromise everyone would be OK with. If
anyone have any other suggestions -- I'm all ears.

Andrey Smirnov

P.S. Just for the future reference, because English is not my first
language I expect some of the ChangeLog messages to be awkwardly
phrased. If that's the case feel free to nudge me about it and I'll
correct it(please do provide suggestions for correction).

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

[Tom Tromey]

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

Andrey> I changed(after it was pointed out that they do not conform to GNU
Andrey> policy) the style of my ChangeLog entries to
Andrey>         * 42.c (trillian): Rename zaphod to beeblebrox(-Wshadow).
Andrey> Hope this is a reasonable compromise everyone would be OK with. If
Andrey> anyone have any other suggestions -- I'm all ears.

I think that is fine.

Tom

[PATCH 022/238] [misc.] bcache.c: -Wshadow fix.

[Andrey Smirnov]

Cause:
        Clashes with `bcache' function from "bcache.h".

To ChangeLog:
	* bcache.c (expand_hash_table): Rename `bcache' to `cache'(-Wshadow).
---
 gdb/ChangeLog |    5 +++++
 gdb/bcache.c  |   30 +++++++++++++++---------------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4ce5f86..57ee41f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
 
+	* bcache.c (expand_hash_table): Fix -Wshadow
+	warnings.
+
+2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
+
 	* annotate.c (annotate_array_section_begin): Fix -Wshadow
 	warnings.
 
diff --git a/gdb/bcache.c b/gdb/bcache.c
index 76e3893..5c287d9 100644
--- a/gdb/bcache.c
+++ b/gdb/bcache.c
@@ -130,7 +130,7 @@ hash_continue (const void *addr, int length, unsigned long h)
 #define CHAIN_LENGTH_THRESHOLD (5)
 
 static void
-expand_hash_table (struct bcache *bcache)
+expand_hash_table (struct bcache *cache)
 {
   /* A table of good hash table sizes.  Whenever we grow, we pick the
      next larger size from this table.  sizes[i] is close to 1 << (i+10),
@@ -149,13 +149,13 @@ expand_hash_table (struct bcache *bcache)
 
   /* Count the stats.  Every unique item needs to be re-hashed and
      re-entered.  */
-  bcache->expand_count++;
-  bcache->expand_hash_count += bcache->unique_count;
+  cache->expand_count++;
+  cache->expand_hash_count += cache->unique_count;
 
   /* Find the next size.  */
-  new_num_buckets = bcache->num_buckets * 2;
+  new_num_buckets = cache->num_buckets * 2;
   for (i = 0; i < (sizeof (sizes) / sizeof (sizes[0])); i++)
-    if (sizes[i] > bcache->num_buckets)
+    if (sizes[i] > cache->num_buckets)
       {
 	new_num_buckets = sizes[i];
 	break;
@@ -168,22 +168,22 @@ expand_hash_table (struct bcache *bcache)
     new_buckets = (struct bstring **) xmalloc (new_size);
     memset (new_buckets, 0, new_size);
 
-    bcache->structure_size -= (bcache->num_buckets
-			       * sizeof (bcache->bucket[0]));
-    bcache->structure_size += new_size;
+    cache->structure_size -= (cache->num_buckets
+			       * sizeof (cache->bucket[0]));
+    cache->structure_size += new_size;
   }
 
   /* Rehash all existing strings.  */
-  for (i = 0; i < bcache->num_buckets; i++)
+  for (i = 0; i < cache->num_buckets; i++)
     {
       struct bstring *s, *next;
 
-      for (s = bcache->bucket[i]; s; s = next)
+      for (s = cache->bucket[i]; s; s = next)
 	{
 	  struct bstring **new_bucket;
 	  next = s->next;
 
-	  new_bucket = &new_buckets[(bcache->hash_function (&s->d.data,
+	  new_bucket = &new_buckets[(cache->hash_function (&s->d.data,
 							    s->length)
 				     % new_num_buckets)];
 	  s->next = *new_bucket;
@@ -192,10 +192,10 @@ expand_hash_table (struct bcache *bcache)
     }
 
   /* Plug in the new table.  */
-  if (bcache->bucket)
-    xfree (bcache->bucket);
-  bcache->bucket = new_buckets;
-  bcache->num_buckets = new_num_buckets;
+  if (cache->bucket)
+    xfree (cache->bucket);
+  cache->bucket = new_buckets;
+  cache->num_buckets = new_num_buckets;
 }
 
 \f
-- 
1.7.5.4

[PATCH 026/238] [misc.] bcache.c: -Wshadow fix

[Andrey Smirnov]

Cause:
        `bcache' from "bcache.h"

To ChangeLog:
	* bcache.c (bcache_memory_used): Rename `bcache' to `cache'(-Wshadow).
---
 gdb/ChangeLog |    5 +++++
 gdb/bcache.c  |    6 +++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8a0adc0..523da0f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
 
+	* bcache.c (bcache_memory_used): Fix -Wshadow
+	warnings.
+
+2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
+
 	* bcache.c (bcache_xfree): Fix -Wshadow
 	warnings.
 
diff --git a/gdb/bcache.c b/gdb/bcache.c
index 6cd64f5..5c82f58 100644
--- a/gdb/bcache.c
+++ b/gdb/bcache.c
@@ -488,9 +488,9 @@ Total memory used by bcache, including overhead: %ld\n"),
 }
 
 int
-bcache_memory_used (struct bcache *bcache)
+bcache_memory_used (struct bcache *cache)
 {
-  if (bcache->total_count == 0)
+  if (cache->total_count == 0)
     return 0;
-  return obstack_memory_used (&bcache->cache);
+  return obstack_memory_used (&cache->cache);
 }
-- 
1.7.5.4

Re: [PATCH 026/238] [misc.] bcache.c: -Wshadow fix

[Tom Tromey]

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

Andrey> 	* bcache.c (bcache_memory_used): Rename `bcache' to `cache'(-Wshadow).

Ok.

Tom

[PATCH 024/238] [misc.] bcache.c: -Wshadow fix

[Andrey Smirnov]

Cause:
        `bcache' function from "bcache.h"

To ChangeLog:
	* bcache.c (bcache_full): Rename `bcache' to `cache'(-Wshadow).
---
 gdb/ChangeLog |    5 +++++
 gdb/bcache.c  |   36 ++++++++++++++++++------------------
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 431e4f6..f23767c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
 
+	* bcache.c (bcache_full): Fix -Wshadow
+	warnings.
+
+2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
+
 	* bcache.c (bcache): Fix -Wshadow
 	warnings.
 
diff --git a/gdb/bcache.c b/gdb/bcache.c
index 035ed22..6e51cf2 100644
--- a/gdb/bcache.c
+++ b/gdb/bcache.c
@@ -221,7 +221,7 @@ bcache (const void *addr, int length, struct bcache *cache)
    returning an old entry.  */
 
 const void *
-bcache_full (const void *addr, int length, struct bcache *bcache, int *added)
+bcache_full (const void *addr, int length, struct bcache *cache, int *added)
 {
   unsigned long full_hash;
   unsigned short half_hash;
@@ -233,55 +233,55 @@ bcache_full (const void *addr, int length, struct bcache *bcache, int *added)
 
   /* Lazily initialize the obstack.  This can save quite a bit of
      memory in some cases.  */
-  if (bcache->total_count == 0)
+  if (cache->total_count == 0)
     {
       /* We could use obstack_specify_allocation here instead, but
 	 gdb_obstack.h specifies the allocation/deallocation
 	 functions.  */
-      obstack_init (&bcache->cache);
+      obstack_init (&cache->cache);
     }
 
   /* If our average chain length is too high, expand the hash table.  */
-  if (bcache->unique_count >= bcache->num_buckets * CHAIN_LENGTH_THRESHOLD)
-    expand_hash_table (bcache);
+  if (cache->unique_count >= cache->num_buckets * CHAIN_LENGTH_THRESHOLD)
+    expand_hash_table (cache);
 
-  bcache->total_count++;
-  bcache->total_size += length;
+  cache->total_count++;
+  cache->total_size += length;
 
-  full_hash = bcache->hash_function (addr, length);
+  full_hash = cache->hash_function (addr, length);
 
   half_hash = (full_hash >> 16);
-  hash_index = full_hash % bcache->num_buckets;
+  hash_index = full_hash % cache->num_buckets;
 
   /* Search the hash bucket for a string identical to the caller's.
      As a short-circuit first compare the upper part of each hash
      values.  */
-  for (s = bcache->bucket[hash_index]; s; s = s->next)
+  for (s = cache->bucket[hash_index]; s; s = s->next)
     {
       if (s->half_hash == half_hash)
 	{
 	  if (s->length == length
-	      && bcache->compare_function (&s->d.data, addr, length))
+	      && cache->compare_function (&s->d.data, addr, length))
 	    return &s->d.data;
 	  else
-	    bcache->half_hash_miss_count++;
+	    cache->half_hash_miss_count++;
 	}
     }
 
   /* The user's string isn't in the list.  Insert it after *ps.  */
   {
     struct bstring *new
-      = obstack_alloc (&bcache->cache, BSTRING_SIZE (length));
+      = obstack_alloc (&cache->cache, BSTRING_SIZE (length));
 
     memcpy (&new->d.data, addr, length);
     new->length = length;
-    new->next = bcache->bucket[hash_index];
+    new->next = cache->bucket[hash_index];
     new->half_hash = half_hash;
-    bcache->bucket[hash_index] = new;
+    cache->bucket[hash_index] = new;
 
-    bcache->unique_count++;
-    bcache->unique_size += length;
-    bcache->structure_size += BSTRING_SIZE (length);
+    cache->unique_count++;
+    cache->unique_size += length;
+    cache->structure_size += BSTRING_SIZE (length);
 
     if (added)
       *added = 1;
-- 
1.7.5.4

Re: [PATCH 024/238] [misc.] bcache.c: -Wshadow fix

[Tom Tromey]

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

Andrey> 	* bcache.c (bcache_full): Rename `bcache' to `cache'(-Wshadow).

Ok.

Tom

[PATCH 025/238] [misc.] bcache.c: -Wshadow fix

[Andrey Smirnov]

Cause:
        `bcache' from "bcache.h"

To ChangeLog:
	* bcache.c (bcache_xfree): Rename `bcache' to `cache'(-Wshadow).
---
 gdb/ChangeLog |    5 +++++
 gdb/bcache.c  |   12 ++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f23767c..8a0adc0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
 
+	* bcache.c (bcache_xfree): Fix -Wshadow
+	warnings.
+
+2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
+
 	* bcache.c (bcache_full): Fix -Wshadow
 	warnings.
 
diff --git a/gdb/bcache.c b/gdb/bcache.c
index 6e51cf2..6cd64f5 100644
--- a/gdb/bcache.c
+++ b/gdb/bcache.c
@@ -330,15 +330,15 @@ bcache_xmalloc (unsigned long (*hash_function)(const void *, int length),
 
 /* Free all the storage associated with BCACHE.  */
 void
-bcache_xfree (struct bcache *bcache)
+bcache_xfree (struct bcache *cache)
 {
-  if (bcache == NULL)
+  if (cache == NULL)
     return;
   /* Only free the obstack if we actually initialized it.  */
-  if (bcache->total_count > 0)
-    obstack_free (&bcache->cache, 0);
-  xfree (bcache->bucket);
-  xfree (bcache);
+  if (cache->total_count > 0)
+    obstack_free (&cache->cache, 0);
+  xfree (cache->bucket);
+  xfree (cache);
 }
 
 
-- 
1.7.5.4

Re: [PATCH 025/238] [misc.] bcache.c: -Wshadow fix

[Tom Tromey]

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

Andrey> 	* bcache.c (bcache_xfree): Rename `bcache' to `cache'(-Wshadow).

Ok.

Tom

Re: [PATCH 022/238] [misc.] bcache.c: -Wshadow fix.

[Tom Tromey]

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

Andrey> 	* bcache.c (expand_hash_table): Rename `bcache' to `cache'(-Wshadow).

Ok.

For the record, I am approving the ones I think are useful.
I still think that the best course is to use -Wshadow in conjunction
with a newer GCC; and to implement configury to check for this GCC
feature.

Tom

Re: [PATCH 022/238] [misc.] bcache.c: -Wshadow fix.

[Andrey Smirnov]

On Tue, Dec 20, 2011 at 7:41 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Andrey" == Andrey Smirnov <andrew.smirnov@gmail.com> writes:
>
> Andrey>         * bcache.c (expand_hash_table): Rename `bcache' to `cache'(-Wshadow).
>
> Ok.
>

Thanks for reviewing the patch. I think I'm going to postpone applying
it, since that what I told Joel I would do.

> For the record, I am approving the ones I think are useful.
> I still think that the best course is to use -Wshadow in conjunction
> with a newer GCC; and to implement configury to check for this GCC
> feature.

I washed my hands of the issue, so I won't be sending patches
implementing said configury or any code to that effect anytime soon, but
I think that statement for the record will probably get lost among all
the messages on -Wshadow topic, and you're going to have to repeat
yourself once the issue is brought up again in the future :)

Andrey Smirnov

Re: [PATCH 022/238] [misc.] bcache.c: -Wshadow fix.

[Tom Tromey]

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

Andrey> Thanks for reviewing the patch. I think I'm going to postpone applying
Andrey> it, since that what I told Joel I would do.

Ok.

Tom> For the record, I am approving the ones I think are useful.
Tom> I still think that the best course is to use -Wshadow in conjunction
Tom> with a newer GCC; and to implement configury to check for this GCC
Tom> feature.

Andrey> I washed my hands of the issue, so I won't be sending patches
Andrey> implementing said configury or any code to that effect anytime soon, but
Andrey> I think that statement for the record will probably get lost among all
Andrey> the messages on -Wshadow topic, and you're going to have to repeat
Andrey> yourself once the issue is brought up again in the future :)

Ok, I see.
Does that mean you are abandoning the whole series?

Tom

Re: [PATCH 022/238] [misc.] bcache.c: -Wshadow fix.

[Andrey Smirnov]

> Tom> For the record, I am approving the ones I think are useful.
> Tom> I still think that the best course is to use -Wshadow in conjunction
> Tom> with a newer GCC; and to implement configury to check for this GCC
> Tom> feature.
>
> Andrey> I washed my hands of the issue, so I won't be sending patches
> Andrey> implementing said configury or any code to that effect anytime soon, but
> Andrey> I think that statement for the record will probably get lost among all
> Andrey> the messages on -Wshadow topic, and you're going to have to repeat
> Andrey> yourself once the issue is brought up again in the future :)
>
> Ok, I see.
> Does that mean you are abandoning the whole series?

No, not at all, just the arguing on whether -Wshadow should be enabled
by default or not part.

Andrey Smirnov

Re: [PATCH 022/238] [misc.] bcache.c: -Wshadow fix.

[Tom Tromey]

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

Andrey> No, not at all, just the arguing on whether -Wshadow should be enabled
Andrey> by default or not part.

Ok, I see, thanks.

Tom