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
Attachments:
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
Attachments:
pg_upgrade.difftext/x-diff; charset=us-asciiDownload
diff --git a/contrib/pg_upgrade_support/pg_upgrade_support.c b/contrib/pg_upgrade_support/pg_upgrade_support.c
new file mode 100644
index 99e64c4..6e30deb
*** a/contrib/pg_upgrade_support/pg_upgrade_support.c
--- b/contrib/pg_upgrade_support/pg_upgrade_support.c
***************
*** 11,16 ****
--- 11,17 ----
#include "postgres.h"
+ #include "catalog/binary_upgrade.h"
#include "catalog/namespace.h"
#include "catalog/pg_type.h"
#include "commands/extension.h"
***************
*** 24,40 ****
PG_MODULE_MAGIC;
#endif
- extern PGDLLIMPORT Oid binary_upgrade_next_pg_type_oid;
- extern PGDLLIMPORT Oid binary_upgrade_next_array_pg_type_oid;
- extern PGDLLIMPORT Oid binary_upgrade_next_toast_pg_type_oid;
-
- extern PGDLLIMPORT Oid binary_upgrade_next_heap_pg_class_oid;
- extern PGDLLIMPORT Oid binary_upgrade_next_index_pg_class_oid;
- extern PGDLLIMPORT Oid binary_upgrade_next_toast_pg_class_oid;
-
- extern PGDLLIMPORT Oid binary_upgrade_next_pg_enum_oid;
- extern PGDLLIMPORT Oid binary_upgrade_next_pg_authid_oid;
-
Datum set_next_pg_type_oid(PG_FUNCTION_ARGS);
Datum set_next_array_pg_type_oid(PG_FUNCTION_ARGS);
Datum set_next_toast_pg_type_oid(PG_FUNCTION_ARGS);
--- 25,30 ----
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
new file mode 100644
index 6f2e142..032a20e
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
***************
*** 34,39 ****
--- 34,40 ----
#include "access/sysattr.h"
#include "access/transam.h"
#include "access/xact.h"
+ #include "catalog/binary_upgrade.h"
#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/heap.h"
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
new file mode 100644
index aa31429..7ad9720
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***************
*** 30,35 ****
--- 30,36 ----
#include "access/visibilitymap.h"
#include "access/xact.h"
#include "bootstrap/bootstrap.h"
+ #include "catalog/binary_upgrade.h"
#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/heap.h"
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
new file mode 100644
index 35899b4..23d2a41
*** a/src/backend/catalog/pg_enum.c
--- b/src/backend/catalog/pg_enum.c
***************
*** 17,22 ****
--- 17,23 ----
#include "access/heapam.h"
#include "access/htup_details.h"
#include "access/xact.h"
+ #include "catalog/binary_upgrade.h"
#include "catalog/catalog.h"
#include "catalog/indexing.h"
#include "catalog/pg_enum.h"
diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
new file mode 100644
index 23ac3dd..634915b
*** a/src/backend/catalog/pg_type.c
--- b/src/backend/catalog/pg_type.c
***************
*** 17,22 ****
--- 17,23 ----
#include "access/heapam.h"
#include "access/htup_details.h"
#include "access/xact.h"
+ #include "catalog/binary_upgrade.h"
#include "catalog/dependency.h"
#include "catalog/indexing.h"
#include "catalog/objectaccess.h"
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
new file mode 100644
index 385d64d..f58e434
*** a/src/backend/catalog/toasting.c
--- b/src/backend/catalog/toasting.c
***************
*** 16,21 ****
--- 16,22 ----
#include "access/tuptoaster.h"
#include "access/xact.h"
+ #include "catalog/binary_upgrade.h"
#include "catalog/dependency.h"
#include "catalog/heap.h"
#include "catalog/index.h"
***************
*** 31,38 ****
#include "utils/syscache.h"
/* Potentially set by contrib/pg_upgrade_support functions */
- extern Oid binary_upgrade_next_toast_pg_class_oid;
-
Oid binary_upgrade_next_toast_pg_type_oid = InvalidOid;
static bool create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
--- 32,37 ----
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
new file mode 100644
index d4a14ca..959b3f2
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
***************
*** 35,40 ****
--- 35,41 ----
#include "access/heapam.h"
#include "access/htup_details.h"
#include "access/xact.h"
+ #include "catalog/binary_upgrade.h"
#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/heap.h"
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
new file mode 100644
index e101a86..ca1906d
*** a/src/backend/commands/user.c
--- b/src/backend/commands/user.c
***************
*** 16,21 ****
--- 16,22 ----
#include "access/heapam.h"
#include "access/htup_details.h"
#include "access/xact.h"
+ #include "catalog/binary_upgrade.h"
#include "catalog/dependency.h"
#include "catalog/indexing.h"
#include "catalog/objectaccess.h"
diff --git a/src/include/catalog/binary_upgrade.h b/src/include/catalog/binary_upgrade.h
new file mode 100644
index ...169d769
*** a/src/include/catalog/binary_upgrade.h
--- b/src/include/catalog/binary_upgrade.h
***************
*** 0 ****
--- 1,29 ----
+ /*-------------------------------------------------------------------------
+ *
+ * binary_upgrade.h
+ * variables used for binary upgrades
+ *
+ *
+ * Portions Copyright (c) 1996-2013, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/catalog/binary_upgrade.h
+ *
+ *-------------------------------------------------------------------------
+ */
+ #ifndef BINARY_UPGRADE_H
+ #define BINARY_UPGRADE_H
+
+ extern PGDLLIMPORT Oid binary_upgrade_next_pg_type_oid;
+ extern PGDLLIMPORT Oid binary_upgrade_next_array_pg_type_oid;
+ extern PGDLLIMPORT Oid binary_upgrade_next_toast_pg_type_oid;
+
+ extern PGDLLIMPORT Oid binary_upgrade_next_heap_pg_class_oid;
+ extern PGDLLIMPORT Oid binary_upgrade_next_index_pg_class_oid;
+ extern PGDLLIMPORT Oid binary_upgrade_next_toast_pg_class_oid;
+
+ extern PGDLLIMPORT Oid binary_upgrade_next_pg_enum_oid;
+ extern PGDLLIMPORT Oid binary_upgrade_next_pg_authid_oid;
+
+ #endif /* BINARY_UPGRADE_H */
+
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
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
Attachments:
0001-Mark-some-more-variables-as-static-or-include-the-ap.patchtext/x-patch; charset=us-asciiDownload
>From a7c6d7b2f5d6d61a302adadb841926cd27582843 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 20 Dec 2013 00:06:17 +0100
Subject: [PATCH] Mark some more variables as static or include the appropriate
header.
Detected by clang's -Wmissing-variable-declarations.
---
src/backend/commands/event_trigger.c | 2 +-
src/backend/postmaster/bgworker.c | 2 +-
src/backend/postmaster/postmaster.c | 3 +--
src/backend/storage/lmgr/s_lock.c | 1 +
src/backend/utils/init/globals.c | 1 +
src/bin/initdb/initdb.c | 2 +-
src/include/storage/pg_shmem.h | 2 +-
src/interfaces/ecpg/preproc/pgc.l | 2 +-
8 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 328e2a8..1164199 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -51,7 +51,7 @@ typedef struct EventTriggerQueryState
struct EventTriggerQueryState *previous;
} EventTriggerQueryState;
-EventTriggerQueryState *currentEventTriggerState = NULL;
+static EventTriggerQueryState *currentEventTriggerState = NULL;
typedef struct
{
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index bca2380..7f02294 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -90,7 +90,7 @@ struct BackgroundWorkerHandle
uint64 generation;
};
-BackgroundWorkerArray *BackgroundWorkerData;
+static BackgroundWorkerArray *BackgroundWorkerData;
/*
* Calculate shared memory needed.
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 048a189..5580489 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -238,8 +238,6 @@ bool enable_bonjour = false;
char *bonjour_name;
bool restart_after_crash = true;
-char *output_config_variable = NULL;
-
/* PIDs of special child processes; 0 when not running */
static pid_t StartupPID = 0,
BgWriterPID = 0,
@@ -545,6 +543,7 @@ PostmasterMain(int argc, char *argv[])
char *userDoption = NULL;
bool listen_addr_saved = false;
int i;
+ char *output_config_variable = NULL;
MyProcPid = PostmasterPid = getpid();
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 138b337..0dad679 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -19,6 +19,7 @@
#include <unistd.h>
#include "storage/s_lock.h"
+#include "storage/barrier.h"
slock_t dummy_spinlock;
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index dd1309b..db832fa 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -18,6 +18,7 @@
*/
#include "postgres.h"
+#include "libpq/libpq-be.h"
#include "libpq/pqcomm.h"
#include "miscadmin.h"
#include "storage/backendid.h"
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 964d284..83fdc88 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -182,7 +182,7 @@ static const char *backend_options = "--single -F -O -c search_path=pg_catalog -
#ifdef WIN32
char *restrict_env;
#endif
-const char *subdirs[] = {
+static const char *subdirs[] = {
"global",
"pg_xlog",
"pg_xlog/archive_status",
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
index 251fbdf..8959299 100644
--- a/src/include/storage/pg_shmem.h
+++ b/src/include/storage/pg_shmem.h
@@ -39,7 +39,6 @@ typedef struct PGShmemHeader /* standard header for all Postgres shmem */
} PGShmemHeader;
-#ifdef EXEC_BACKEND
#ifndef WIN32
extern unsigned long UsedShmemSegID;
#else
@@ -47,6 +46,7 @@ extern HANDLE UsedShmemSegID;
#endif
extern void *UsedShmemSegAddr;
+#ifdef EXEC_BACKEND
extern void PGSharedMemoryReAttach(void);
#endif
diff --git a/src/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l
index f04e34a..69a0027 100644
--- a/src/interfaces/ecpg/preproc/pgc.l
+++ b/src/interfaces/ecpg/preproc/pgc.l
@@ -56,7 +56,7 @@ static bool isdefine(void);
static bool isinformixdefine(void);
char *token_start;
-int state_before;
+static int state_before;
struct _yy_buffer
{
--
1.8.3.251.g1462b67
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers