Thread: 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
Attachment
Andres Freund <andres@2ndquadrant.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@2ndquadrant.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Attachment
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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.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
Attachment
On 2013-12-19 14:56:38 -0800, Kevin Grittner wrote: > Bruce Momjian <bruce@momjian.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
Attachment
Re: clang's -Wmissing-variable-declarations shows some shoddy programming
From
Peter Eisentraut
Date:
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