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

On Sat, Jan 21, 2012 at 1:06 PM, Peter Eisentraut <peter_e(at)gmx(dot)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