Horton

Re: consider -Wmissing-variable-declarations

Peter Eisentraut <peter@eisentraut.org>

View thread View on postgr.es
From: Peter Eisentraut <peter@eisentraut.org>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>, Heikki Linnakangas <hlinnaka@iki.fi>
Date: 2024-07-02T06:30:49Z
Lists: pgsql-hackers 
I have committed the first few of these.  (The compiler warning flag 
itself is not activated yet.)  This should allow you to proceed with 
your patches that add various const qualifiers.  I'll come back to the 
rest later.


On 18.06.24 17:02, Andres Freund wrote:
>> diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
>> index 07bf356b70c..5a124385b7c 100644
>> --- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
>> +++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
>> @@ -19,17 +19,18 @@
>>   #include "common/logging.h"
>>   #include "getopt_long.h"
>>   
>> -const char *progname;
>> +static const char *progname;
> 
> Hm, this one I'm not so sure about. The backend version is explicitly globally
> visible, and I don't see why we shouldn't do the same for other binaries.

We have in various programs a mix of progname with static linkage and 
with external linkage.  AFAICT, this is merely determined by whether 
there are multiple source files that need it, not by some higher-level 
scheme.

>>  From d89312042eb76c879d699380a5e2ed0bc7956605 Mon Sep 17 00:00:00 2001
>> From: Peter Eisentraut <peter@eisentraut.org>
>> Date: Sun, 16 Jun 2024 23:52:06 +0200
>> Subject: [PATCH v2 05/10] Fix warnings from -Wmissing-variable-declarations
>>   under EXEC_BACKEND
>>
>> The NON_EXEC_STATIC variables need a suitable declaration in a header
>> file under EXEC_BACKEND.
>>
>> Also fix the inconsistent application of the volatile qualifier for
>> PMSignalState, which was revealed by this change.
> 
> I'm very very unenthused about adding volatile to more places. It's rarely
> correct and often slow. But I guess this doesn't really make it any worse.

Yeah, it's not always clear with volatile, but in this one case it's 
probably better to keep it consistent rather than having to cast it away 
or something.

>> +#ifdef TRACE_SYNCSCAN
>> +#include "access/syncscan.h"
>> +#endif
> 
> I'd just include it unconditionally.

My thinking here was that if we apply an include file cleaner (like 
iwyu) sometime, it would flag this include as unused.  This way it's 
clearer what it's for.

Commits

  1. Use CXXFLAGS instead of CFLAGS for linking C++ code

  2. Add -Wmissing-variable-declarations to the standard compilation flags

  3. Include bison header files into implementation files

  4. Fix -Wmissing-variable-declarations warnings for float.c special case

  5. Add extern declarations for Bison global variables

  6. Move all extern declarations for GUC variables to header files

  7. Move extern declarations for EXEC_BACKEND to header files

  8. Get rid of a global variable

  9. Add missing includes for some global variables

  10. Convert some extern variables to static

  11. Improve some global variable declarations

  12. Remove useless extern keywords