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.

Search Discussions

  • Robert Haas at Mar 30, 2011 at 2:57 pm

    On Tue, Mar 29, 2011 at 4:48 PM, 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.

    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
  • Peter Eisentraut at Apr 27, 2011 at 5:15 pm

    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.
  • Magnus Hagander at Apr 27, 2011 at 5:39 pm

    On Wed, Apr 27, 2011 at 18:55, Peter Eisentraut 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... ;)
  • Peter Eisentraut at Apr 27, 2011 at 6:21 pm

    On ons, 2011-04-27 at 19:17 +0200, Magnus Hagander wrote:
    On Wed, Apr 27, 2011 at 18:55, Peter Eisentraut 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.
  • Magnus Hagander at Apr 27, 2011 at 6:31 pm

    On Wed, Apr 27, 2011 at 20:21, Peter Eisentraut wrote:
    On ons, 2011-04-27 at 19:17 +0200, Magnus Hagander wrote:
    On Wed, Apr 27, 2011 at 18:55, Peter Eisentraut 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.
  • Peter Eisentraut at Apr 27, 2011 at 5:23 pm

    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedMar 29, '11 at 8:49p
activeApr 27, '11 at 6:31p
posts7
users3
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase