Обсуждение: gcc 4.6 warnings -Wunused-but-set-variable

As you might have heard, GCC 4.6 was released the other day.  It
generates a bunch of new warnings with the PostgreSQL source code, most
of which belong to the new warning scenario -Wunused-but-set-variable,
which is included in -Wall.

Attached is a patch that gets rid of most of these.  As you can see,
most of these remove real leftover garbage.  The line I marked in
pg_basebackup.c might be an actual problem: It goes through a whole lot
to figure out the timeline and then doesn't do anything with it.  In
some other cases, however, one might argue that the changes lose some
clarity, such as when dropping the return value of strtoul() or
va_arg().  How should we proceed?  In any case, my patch should be
re-reviewed for any possible side effects that I might have hastily
removed.

On Tue, Mar 29, 2011 at 4:48 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> As you might have heard, GCC 4.6 was released the other day.  It
> generates a bunch of new warnings with the PostgreSQL source code, most
> of which belong to the new warning scenario -Wunused-but-set-variable,
> which is included in -Wall.
>
> Attached is a patch that gets rid of most of these.  As you can see,
> most of these remove real leftover garbage.  The line I marked in
> pg_basebackup.c might be an actual problem: It goes through a whole lot
> to figure out the timeline and then doesn't do anything with it.  In
> some other cases, however, one might argue that the changes lose some
> clarity, such as when dropping the return value of strtoul() or
> va_arg().  How should we proceed?  In any case, my patch should be
> re-reviewed for any possible side effects that I might have hastily
> removed.

In the case of variable.c, it is entirely unclear that there's any
point in calling strtoul() at all.  Maybe we should just remove that
and the following Assert() as well.

In parse_utilcmd.c, do we need to look up the collation OID if we're
just discarding it anyway?

In the case of the va_arg() calls, maybe something like /* advance arg
position, but ignore result */?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On tis, 2011-03-29 at 23:48 +0300, Peter Eisentraut wrote:
> The line I marked in pg_basebackup.c might be an actual problem: It
> goes through a whole lot to figure out the timeline and then doesn't
> do anything with it.

This hasn't been addressed yet.  It doesn't manifest itself as an actual
problem, but it looks as though someone had intended something in that
code and the code doesn't do that.

On tis, 2011-03-29 at 23:48 +0300, Peter Eisentraut wrote:
> As you might have heard, GCC 4.6 was released the other day.  It
> generates a bunch of new warnings with the PostgreSQL source code, most
> of which belong to the new warning scenario -Wunused-but-set-variable,
> which is included in -Wall.

In case someone else tries that, I have filed a bug with GCC regarding
some of the other warnings:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48778

On Wed, Apr 27, 2011 at 18:55, Peter Eisentraut <peter_e@gmx.net> wrote:
> On tis, 2011-03-29 at 23:48 +0300, Peter Eisentraut wrote:
>> The line I marked in pg_basebackup.c might be an actual problem: It
>> goes through a whole lot to figure out the timeline and then doesn't
>> do anything with it.
>
> This hasn't been addressed yet.  It doesn't manifest itself as an actual
> problem, but it looks as though someone had intended something in that
> code and the code doesn't do that.

Do you have a ref to the actual problem? The subject change killed my
threading, the email was trimmed to not include the actual problem,
and it appears not to be listed on the open items list... ;)


--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On ons, 2011-04-27 at 19:17 +0200, Magnus Hagander wrote:
> On Wed, Apr 27, 2011 at 18:55, Peter Eisentraut <peter_e@gmx.net> wrote:
> > On tis, 2011-03-29 at 23:48 +0300, Peter Eisentraut wrote:
> >> The line I marked in pg_basebackup.c might be an actual problem: It
> >> goes through a whole lot to figure out the timeline and then doesn't
> >> do anything with it.
> >
> > This hasn't been addressed yet.  It doesn't manifest itself as an actual
> > problem, but it looks as though someone had intended something in that
> > code and the code doesn't do that.
> 
> Do you have a ref to the actual problem? The subject change killed my
> threading, the email was trimmed to not include the actual problem,
> and it appears not to be listed on the open items list... ;)

In BaseBackup(), the variable timeline is assigned in a somewhat
elaborate fashion, but then the result is not used for anything.

On Wed, Apr 27, 2011 at 20:21, Peter Eisentraut <peter_e@gmx.net> wrote:
> On ons, 2011-04-27 at 19:17 +0200, Magnus Hagander wrote:
>> On Wed, Apr 27, 2011 at 18:55, Peter Eisentraut <peter_e@gmx.net> wrote:
>> > On tis, 2011-03-29 at 23:48 +0300, Peter Eisentraut wrote:
>> >> The line I marked in pg_basebackup.c might be an actual problem: It
>> >> goes through a whole lot to figure out the timeline and then doesn't
>> >> do anything with it.
>> >
>> > This hasn't been addressed yet.  It doesn't manifest itself as an actual
>> > problem, but it looks as though someone had intended something in that
>> > code and the code doesn't do that.
>>
>> Do you have a ref to the actual problem? The subject change killed my
>> threading, the email was trimmed to not include the actual problem,
>> and it appears not to be listed on the open items list... ;)
>
> In BaseBackup(), the variable timeline is assigned in a somewhat
> elaborate fashion, but then the result is not used for anything.

Ah, I see it.

What happened there is I accidentally included it when I split my
patches apart. It's required in the "stream WAL in parallel to the
base backup to decrease requirements on wal_keep_segmtents". But that
patch was postponed since there were still bugs in it, and it wasn't
entirely feature-complete, and we were pretty far past feature-freeze.

So it's not needed in 9.1. I'll rip it out and move it over to the
patch once it's ready to go for 9.2.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/