Re: clang's -Wmissing-variable-declarations shows some shoddy programming

Hi,

Compiling postgres with said option in CFLAGS really gives an astounding
number of warnings. Except some bison/flex generated ones, none of them
looks acceptable to me.
Most are just file local variables with a missing static and easy to
fix. Several other are actually shared variables, where people simply
haven't bothered to add the variable to a header. Some of them with
comments declaring that fact, others adding longer comments, even others
adding longer comments about that fact.

I've attached the output of such a compilation run for those without
clang.

Greetings,

Andres Freund

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

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> Compiling postgres with said option in CFLAGS really gives an astounding
> number of warnings. Except some bison/flex generated ones, none of them
> looks acceptable to me.

Given that we're not going to be able to get rid of the bison/flex cases,
is this really something to bother with? I agree I don't like cases where
there's an "extern" in some other .c file rather than in a header, but I'm
dubious about making a goal of suppressing this warning as such.

regards, tom lane

On 2013-12-14 12:14:25 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > Compiling postgres with said option in CFLAGS really gives an astounding
> > number of warnings. Except some bison/flex generated ones, none of them
> > looks acceptable to me.
>
> Given that we're not going to be able to get rid of the bison/flex cases,
> is this really something to bother with?

On a second look, it's not that hard to supress the warnings for
those. Something *roughly* like:

/*
* Declare variables defined by bison as extern, so clang doesn't complain
* about undeclared non-static variables.
*/
extern int plpgsql_yychar;
extern int plpgsql_yynerrs;

works.

> I agree I don't like cases where
> there's an "extern" in some other .c file rather than in a header, but I'm
> dubious about making a goal of suppressing this warning as such.

The cases where a 'static' is missing imo are cases that should clearly
be fixed, there's just no excuse for them. But it's an easy mistake to
make so having the compiler's support imo is helpful.

WRT the externs in .c files, if it were just old code, I wouldn't
bother. But we're regularly adding them. The last ones just last week in
316472146 and ef3267523 and several others aren't much older. So making
it a policy that can relatively easily be checked automatically not to
do so seems like a good idea.

Unfortunately gcc doesn't have a equivalent warning...

Greetings,

Andres Freund

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

On Sat, Dec 14, 2013 at 04:52:28PM +0100, Andres Freund wrote:
> Hi,
>
> Compiling postgres with said option in CFLAGS really gives an astounding
> number of warnings. Except some bison/flex generated ones, none of them
> looks acceptable to me.
> Most are just file local variables with a missing static and easy to
> fix. Several other are actually shared variables, where people simply
> haven't bothered to add the variable to a header. Some of them with
> comments declaring that fact, others adding longer comments, even others
> adding longer comments about that fact.
>
> I've attached the output of such a compilation run for those without
> clang.

Now that pg_upgrade has stabilized, I think it is time to centralize all
the pg_upgrade_support control variables in a single C include file that
can be used by the backend and by pg_upgrade_support. This will
eliminate the compiler warnings too.

The attached patch accomplishes this.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Hi,

On 2013-12-18 22:11:03 -0500, Bruce Momjian wrote:
> Now that pg_upgrade has stabilized, I think it is time to centralize all
> the pg_upgrade_support control variables in a single C include file that
> can be used by the backend and by pg_upgrade_support. This will
> eliminate the compiler warnings too.

Btw, I think it's more or less lucky the current state works at all -
there's missing PGDLLIMPORT statements on the builtin side...

Greetings,

Andres Freund

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

On Wed, Dec 18, 2013 at 10:11:03PM -0500, Bruce Momjian wrote:
> On Sat, Dec 14, 2013 at 04:52:28PM +0100, Andres Freund wrote:
> > Hi,
> >
> > Compiling postgres with said option in CFLAGS really gives an astounding
> > number of warnings. Except some bison/flex generated ones, none of them
> > looks acceptable to me.
> > Most are just file local variables with a missing static and easy to
> > fix. Several other are actually shared variables, where people simply
> > haven't bothered to add the variable to a header. Some of them with
> > comments declaring that fact, others adding longer comments, even others
> > adding longer comments about that fact.
> >
> > I've attached the output of such a compilation run for those without
> > clang.
>
> Now that pg_upgrade has stabilized, I think it is time to centralize all
> the pg_upgrade_support control variables in a single C include file that
> can be used by the backend and by pg_upgrade_support. This will
> eliminate the compiler warnings too.
>
> The attached patch accomplishes this.

Patch applied.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Sat, Dec 14, 2013 at 04:52:28PM +0100, Andres Freund wrote:
> Hi,
>
> Compiling postgres with said option in CFLAGS really gives an astounding
> number of warnings. Except some bison/flex generated ones, none of them
> looks acceptable to me.
> Most are just file local variables with a missing static and easy to
> fix. Several other are actually shared variables, where people simply
> haven't bothered to add the variable to a header. Some of them with
> comments declaring that fact, others adding longer comments, even others
> adding longer comments about that fact.
>
> I've attached the output of such a compilation run for those without
> clang.

I have fixed the binary_upgrade_* variables defines, and Heikki has
fixed some other cases. Can you rerun the test against git head and
post the updated output? Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> wrote:

> I have fixed the binary_upgrade_* variables defines, and Heikki has
> fixed some other cases.  Can you rerun the test against git head and
> post the updated output?  Thanks.

I'm now seeing the attached.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-12-19 14:56:38 -0800, Kevin Grittner wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> > I have fixed the binary_upgrade_* variables defines, and Heikki has
> > fixed some other cases.  Can you rerun the test against git head and
> > post the updated output?  Thanks.
>
> I'm now seeing the attached.

Heh, too fast for me. I was just working on a patch to fix some of these
;)

The attached patch fixes some of the easiest cases, where either an
include was missing o a variable should have been static.

Greetings,

Andres Freund

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

On Fri, 2013-12-20 at 00:09 +0100, Andres Freund wrote:
> The attached patch fixes some of the easiest cases, where either an
> include was missing o a variable should have been static.

committed