Horton

Thread

  1. lots of unused variable warnings in assert-free builds

    Peter Eisentraut <peter_e@gmx.net> — 2012-01-14T21:53:18Z

    In builds without --enable-cassert (I guess not many developers use
those a lot), there are quite a few unused variable warnings.  These
usually hold some intermediate result that the assert checks later.  I
see that in some places our code already uses #ifdef
USE_ASSERT_CHECKING, presumably to hide similar issues.  But in most
cases using this would significantly butcher the code.  I found that
adding __attribute__((unused)) is cleaner.  Attached is a patch that
cleans up all the warnings I encountered.

  2. Re: lots of unused variable warnings in assert-free builds

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-15T06:37:59Z

    Peter Eisentraut <peter_e@gmx.net> writes:
> I see that in some places our code already uses #ifdef
> USE_ASSERT_CHECKING, presumably to hide similar issues.  But in most
> cases using this would significantly butcher the code.  I found that
> adding __attribute__((unused)) is cleaner.  Attached is a patch that
> cleans up all the warnings I encountered.

Surely this will fail entirely on most non-gcc compilers?  Not to
mention that next month's gcc may complain "hey, you used this 'unused'
variable".  I think #ifdef USE_ASSERT_CHECKING is really the only way
if you care about quieting these warnings.  (Personally, I don't.)

			regards, tom lane

  3. Re: lots of unused variable warnings in assert-free builds

    Andrew Dunstan <andrew@dunslane.net> — 2012-01-15T13:14:52Z

    
On 01/15/2012 01:37 AM, Tom Lane wrote:
> Peter Eisentraut<peter_e@gmx.net>  writes:
>> I see that in some places our code already uses #ifdef
>> USE_ASSERT_CHECKING, presumably to hide similar issues.  But in most
>> cases using this would significantly butcher the code.  I found that
>> adding __attribute__((unused)) is cleaner.  Attached is a patch that
>> cleans up all the warnings I encountered.
> Surely this will fail entirely on most non-gcc compilers?  Not to
> mention that next month's gcc may complain "hey, you used this 'unused'
> variable".  I think #ifdef USE_ASSERT_CHECKING is really the only way
> if you care about quieting these warnings.  (Personally, I don't.)
>
> 			


It would possibly have some documentary value too. Just looking very 
quickly at Peter's patch, I don't really understand his assertion that 
this would significantly butcher the code. The worst effect would be 
that in a few cases we'd have to break up multiple declarations where 
one of the variables was in this class. That doesn't seem like a tragedy.

I like software that compiles in the normal use with few or no warnings. 
I should have thought that would appeal to most packagers, too.

cheers

andrew

  4. Re: lots of unused variable warnings in assert-free builds

    Simon Riggs <simon@2ndquadrant.com> — 2012-01-15T14:02:16Z

    On Sun, Jan 15, 2012 at 1:14 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 01/15/2012 01:37 AM, Tom Lane wrote:
>>
>> Peter Eisentraut<peter_e@gmx.net>  writes:
>>>
>>> I see that in some places our code already uses #ifdef
>>> USE_ASSERT_CHECKING, presumably to hide similar issues.  But in most
>>> cases using this would significantly butcher the code.  I found that
>>> adding __attribute__((unused)) is cleaner.  Attached is a patch that
>>> cleans up all the warnings I encountered.
>>
>> Surely this will fail entirely on most non-gcc compilers?  Not to
>> mention that next month's gcc may complain "hey, you used this 'unused'
>> variable".  I think #ifdef USE_ASSERT_CHECKING is really the only way
>> if you care about quieting these warnings.  (Personally, I don't.)
>>
>>
>
>
>
> It would possibly have some documentary value too. Just looking very quickly
> at Peter's patch, I don't really understand his assertion that this would
> significantly butcher the code. The worst effect would be that in a few
> cases we'd have to break up multiple declarations where one of the variables
> was in this class. That doesn't seem like a tragedy.
>
> I like software that compiles in the normal use with few or no warnings. I
> should have thought that would appeal to most packagers, too.


Sounds good, but let's not do it yet because we have a few patches to
commit first.

It would be good to minimise bit rot during the CF.

-- 
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  5. Re: lots of unused variable warnings in assert-free builds

    Peter Eisentraut <peter_e@gmx.net> — 2012-01-18T19:15:52Z

    On sön, 2012-01-15 at 01:37 -0500, Tom Lane wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
> > I see that in some places our code already uses #ifdef
> > USE_ASSERT_CHECKING, presumably to hide similar issues.  But in most
> > cases using this would significantly butcher the code.  I found that
> > adding __attribute__((unused)) is cleaner.  Attached is a patch that
> > cleans up all the warnings I encountered.
> 
> Surely this will fail entirely on most non-gcc compilers?

No, because __attribute__() is defined to empty for other compilers.  We
use this pattern already.

> Not to
> mention that next month's gcc may complain "hey, you used this 'unused'
> variable".

No, because __attribute__((unused)) means "that the variable is meant to
be possibly unused".

  6. Re: lots of unused variable warnings in assert-free builds

    Peter Eisentraut <peter_e@gmx.net> — 2012-01-18T19:16:56Z

    On sön, 2012-01-15 at 08:14 -0500, Andrew Dunstan wrote:
> It would possibly have some documentary value too. Just looking very 
> quickly at Peter's patch, I don't really understand his assertion that
> this would significantly butcher the code. The worst effect would be 
> that in a few cases we'd have to break up multiple declarations where 
> one of the variables was in this class. That doesn't seem like a
> tragedy.

Well, I'll prepare a patch like that and then you can judge.

  7. Re: lots of unused variable warnings in assert-free builds

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-18T19:21:03Z

    Peter Eisentraut <peter_e@gmx.net> writes:
> On sn, 2012-01-15 at 01:37 -0500, Tom Lane wrote:
>> Surely this will fail entirely on most non-gcc compilers?

> No, because __attribute__() is defined to empty for other compilers.  We
> use this pattern already.

Oh, duh.  In that case my only objection to doing it like this is that
I'd like to see what pgindent will do with it.  pgindent is not very
nice about #ifdef's in variable lists (it tends to insert unwanted
vertical space) so it's possible that this way will look better.

			regards, tom lane

  8. Re: lots of unused variable warnings in assert-free builds

    Peter Eisentraut <peter_e@gmx.net> — 2012-01-21T18:06:15Z

    On ons, 2012-01-18 at 21:16 +0200, Peter Eisentraut wrote:
> On sön, 2012-01-15 at 08:14 -0500, Andrew Dunstan wrote:
> > It would possibly have some documentary value too. Just looking very 
> > quickly at Peter's patch, I don't really understand his assertion that
> > this would significantly butcher the code. The worst effect would be 
> > that in a few cases we'd have to break up multiple declarations where 
> > one of the variables was in this class. That doesn't seem like a
> > tragedy.
> 
> Well, I'll prepare a patch like that and then you can judge.

So, here are three patches that could solve this issue.

pg-cassert-unused-attribute.patch, the one I already showed, using
__attribute__((unused).

pg-cassert-unused-ifdef.patch, using only additional #ifdef
USE_ASSERT_CHECKING.  This does have additional documentation value, but
you can see that it gets bulky and complicated.  (I introduced several
bugs merely while creating this patch.)

pg-cassert-unused-void.patch is an alternative approach that avoids the
warnings by casting the arguments of Assert() to void if assertions are
disabled.  This has the least code impact, but it changes the
traditional semantics of asserts, which is that they disappear
completely when not enabled.  You can see how this creates a problem in
src/backend/replication/syncrep.c, where the nontrivial call to
SyncRepQueueIsOrderedByLSN() now becomes visible even in non-assert
builds.  I played around with some other options like
__attribute__((pure)) to make the compiler optimize the function away in
that case, but that didn't appear to work.  So this might not be a
workable solution (and it would be GCC-specific anyway).

  9. Re: lots of unused variable warnings in assert-free builds

    Robert Haas <robertmhaas@gmail.com> — 2012-01-24T16:52:33Z

    On Sat, Jan 21, 2012 at 1:06 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> So, here are three patches that could solve this issue.
>
> pg-cassert-unused-attribute.patch, the one I already showed, using
> __attribute__((unused).
>
> pg-cassert-unused-ifdef.patch, using only additional #ifdef
> USE_ASSERT_CHECKING.  This does have additional documentation value, but
> you can see that it gets bulky and complicated.  (I introduced several
> bugs merely while creating this patch.)
>
> pg-cassert-unused-void.patch is an alternative approach that avoids the
> warnings by casting the arguments of Assert() to void if assertions are
> disabled.  This has the least code impact, but it changes the
> traditional semantics of asserts, which is that they disappear
> completely when not enabled.  You can see how this creates a problem in
> src/backend/replication/syncrep.c, where the nontrivial call to
> SyncRepQueueIsOrderedByLSN() now becomes visible even in non-assert
> builds.  I played around with some other options like
> __attribute__((pure)) to make the compiler optimize the function away in
> that case, but that didn't appear to work.  So this might not be a
> workable solution (and it would be GCC-specific anyway).

I think the third approach is unacceptable from a performance point of view.

Some of these problems can be fixed without resorting to as much
hackery as you have here.  For example, in nodeWorkTableScan.c, you
could easily fix the problem by getting rid of the estate variable and
replacing its single use within the assertion with the expression from
used to initialize it on the previous line.  I think this might the
cleanest solution in general.

I'm not sure what to do about the cases where that isn't practical.
Spraying the code with __attribute__((unused)) is somewhat undesirable
because it could mask a failure to properly initialize the variable in
an assert-enabled build.  We could have a macro
PG_UNUSED_IF_NO_ASSERTS or something, but that doesn't have any
obvious advantage over just getting rid of the variable altogether in
such cases.  I lean toward the view that variables not needed in
assertion-free builds should be #ifdef'd out altogether, as in your
second patch, but we should try to minimize the number of places where
we need to do that.

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

  10. Re: lots of unused variable warnings in assert-free builds

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-24T17:12:56Z

    Robert Haas <robertmhaas@gmail.com> writes:
> Spraying the code with __attribute__((unused)) is somewhat undesirable
> because it could mask a failure to properly initialize the variable in
> an assert-enabled build.

Ouch.  Is it really true that __attribute__((unused)) disables detection
of use of uninitialized variables?  That would be nasty, and it's not
obvious to me why it should need to work like that.  But if it is true,
then I agree that that makes this approach not too tenable.

			regards, tom lane

  11. Re: lots of unused variable warnings in assert-free builds

    Robert Haas <robertmhaas@gmail.com> — 2012-01-24T17:25:09Z

    On Tue, Jan 24, 2012 at 12:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Spraying the code with __attribute__((unused)) is somewhat undesirable
>> because it could mask a failure to properly initialize the variable in
>> an assert-enabled build.
>
> Ouch.  Is it really true that __attribute__((unused)) disables detection
> of use of uninitialized variables?  That would be nasty, and it's not
> obvious to me why it should need to work like that.  But if it is true,
> then I agree that that makes this approach not too tenable.

Oh, I think maybe I am confused.  The downsides of disabling *unused*
variable detection are obviously much less than the downsides of
disabling *uninitialized* variable declaration... although neither is
ideal.

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

  12. Re: lots of unused variable warnings in assert-free builds

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-24T17:57:36Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jan 24, 2012 at 12:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Ouch. Is it really true that __attribute__((unused)) disables detection
>> of use of uninitialized variables?

> Oh, I think maybe I am confused.  The downsides of disabling *unused*
> variable detection are obviously much less than the downsides of
> disabling *uninitialized* variable declaration... although neither is
> ideal.

OK.  MHO is that __attribute__((unused)) is probably less annoying than
#ifdef overall.  Also, it occurs to me that an intermediate macro
"PG_USED_FOR_ASSERTS_ONLY" would be a good idea, first because it
documents *why* you want to mark the variable as possibly unused,
and second because changing the macro definition would provide an easy way
to check for totally-unused variables, in case we wanted to periodically
make such checks.

This is all modulo the question of what pgindent will do with it,
which I would still like to see tested before we commit to a method.

			regards, tom lane

  13. Re: lots of unused variable warnings in assert-free builds

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-24T18:03:50Z

    I wrote:
> Also, it occurs to me that an intermediate macro
> "PG_USED_FOR_ASSERTS_ONLY" would be a good idea, first because it
> documents *why* you want to mark the variable as possibly unused,
> and second because changing the macro definition would provide an easy way
> to check for totally-unused variables, in case we wanted to periodically
> make such checks.

Uh, wait a second.  Why not

#ifdef USE_ASSERT_CHECKING
#define PG_USED_FOR_ASSERTS_ONLY
#else
#define PG_USED_FOR_ASSERTS_ONLY __attribute__((unused))
#endif

Then, when you build with asserts on, you *automatically* get told
if the variable is entirely unused.

			regards, tom lane

  14. Re: lots of unused variable warnings in assert-free builds

    Robert Haas <robertmhaas@gmail.com> — 2012-01-24T18:08:05Z

    On Tue, Jan 24, 2012 at 1:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Also, it occurs to me that an intermediate macro
>> "PG_USED_FOR_ASSERTS_ONLY" would be a good idea, first because it
>> documents *why* you want to mark the variable as possibly unused,
>> and second because changing the macro definition would provide an easy way
>> to check for totally-unused variables, in case we wanted to periodically
>> make such checks.
>
> Uh, wait a second.  Why not
>
> #ifdef USE_ASSERT_CHECKING
> #define PG_USED_FOR_ASSERTS_ONLY
> #else
> #define PG_USED_FOR_ASSERTS_ONLY __attribute__((unused))
> #endif
>
> Then, when you build with asserts on, you *automatically* get told
> if the variable is entirely unused.

Yes, that's what I meant when I suggested it originally.  I'm just not
sure it's any nicer than adding ifdefs for USE_ASSERT_CHECKING.

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

  15. Re: lots of unused variable warnings in assert-free builds

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-24T18:18:31Z

    Robert Haas <robertmhaas@gmail.com> writes:
> Yes, that's what I meant when I suggested it originally.  I'm just not
> sure it's any nicer than adding ifdefs for USE_ASSERT_CHECKING.

I'm inclined to think that it probably is nicer, just because of less
vertical space used.  But again, this opinion is contingent on what it
will look like after pgindent gets done with it ...

			regards, tom lane

  16. Re: lots of unused variable warnings in assert-free builds

    Peter Eisentraut <peter_e@gmx.net> — 2012-03-20T18:51:59Z

    On tis, 2012-01-24 at 13:18 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > Yes, that's what I meant when I suggested it originally.  I'm just not
> > sure it's any nicer than adding ifdefs for USE_ASSERT_CHECKING.
> 
> I'm inclined to think that it probably is nicer, just because of less
> vertical space used.  But again, this opinion is contingent on what it
> will look like after pgindent gets done with it ...

Here is a demo diff of what pgindent would do with the various
approaches (btw., nice job on making pgindent easy to use for everyone
now).

As you can see, pgindent adds whitespace on top of #ifdef
USE_ASSERT_CHECKING, and messes up the vertical alignment of variable
definitions that contain extra attributes.

All things considered, I like the PG_USED_FOR_ASSERTS_ONLY solution best.

  17. Re: lots of unused variable warnings in assert-free builds

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-03-20T19:04:02Z

    Peter Eisentraut <peter_e@gmx.net> writes:
> As you can see, pgindent adds whitespace on top of #ifdef
> USE_ASSERT_CHECKING, and messes up the vertical alignment of variable
> definitions that contain extra attributes.

Hm.  I bet it thinks that PG_USED_FOR_ASSERTS_ONLY is the variable name,
which means that the behavior might be more exciting for multi-word type
names (for instance "struct foo" or "volatile int *".  Could you check
a few cases like that?

> All things considered, I like the PG_USED_FOR_ASSERTS_ONLY solution best.

I agree, unless the more complicated cases go further off the rails.

			regards, tom lane

  18. Re: lots of unused variable warnings in assert-free builds

    Peter Eisentraut <peter_e@gmx.net> — 2012-03-21T21:34:58Z

    On tis, 2012-03-20 at 15:04 -0400, Tom Lane wrote:
> Hm.  I bet it thinks that PG_USED_FOR_ASSERTS_ONLY is the variable
> name, which means that the behavior might be more exciting for
> multi-word type names (for instance "struct foo" or "volatile int *".
> Could you check a few cases like that?

Tested, doesn't make a difference.  Hence committed that way.