Обсуждение: Enable -Wstrict-prototypes and -Wold-style-definition by default
Hi hackers, PFA a patch to $SUBJECT. The idea is to avoid having to chase those warnings manually like 11171fe1fc8, cdf4b9aff2, 0e72b9d440, 7069dbcc31, f1283ed6cc, 7b66e2c086, e95126cf04 and 9f7c527af3 have done. The idea has been discussed in [1] and [2]. The patch is divided in 2 parts: 0001: Prevent -Wstrict-prototypes and -Wold-style-definition warnings It fixes the remaining warnings that those new flags would generate. 0002: Enable -Wstrict-prototypes and -Wold-style-definition by default Those are available in all gcc and clang versions that support C11 and as C11 is required as of f5e0186f865c, then we can add them without capability test. The new flags are moved in configure.ac late enough to avoid any error (for example, PGAC_PRINTF_ARCHETYPE which uses -Werror and would fail to detect gnu_printf if -Wstrict-prototypes is active) leading to mingw_cross_warning CI task failing. Also, readline headers trigger a lot of warnings with -Wstrict-prototypes, so we make use of the system_header pragma to hide the warnings (as discussed in [3]. [1]: https://postgr.es/m/262909b8-3f4b-42ce-acd7-bdd4a6897990%40eisentraut.org [2]: https://postgr.es/m/aTJ9T8HyJN3D024w%40ip-10-97-1-34.eu-west-3.compute.internal [3]: https://postgr.es/m/1049756.1764861448%40sss.pgh.pa.us Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
On 09.03.26 17:39, Bertrand Drouvot wrote: > 0001: Prevent -Wstrict-prototypes and -Wold-style-definition warnings > > It fixes the remaining warnings that those new flags would generate. I have committed this one. I'll look at the rest next.
On 16.03.26 10:55, Peter Eisentraut wrote: > On 09.03.26 17:39, Bertrand Drouvot wrote: >> 0001: Prevent -Wstrict-prototypes and -Wold-style-definition warnings >> >> It fixes the remaining warnings that those new flags would generate. > > I have committed this one. I'll look at the rest next. Also committed.
Hi, Thank you for working on this! On Wed, 18 Mar 2026 at 16:32, Peter Eisentraut <peter@eisentraut.org> wrote: > > On 16.03.26 10:55, Peter Eisentraut wrote: > > On 09.03.26 17:39, Bertrand Drouvot wrote: > >> 0001: Prevent -Wstrict-prototypes and -Wold-style-definition warnings > >> > >> It fixes the remaining warnings that those new flags would generate. > > > > I have committed this one. I'll look at the rest next. > > Also committed. I got this warning while running headerscheck after this commit: ~/Desktop/projects/postgres/src/interfaces/ecpg/ecpglib/ecpglib_extern.h:221:40: warning: function declaration isn’t a prototype [-Wstrict-prototypes] 221 | void ecpg_init_sqlca(struct sqlca_t *sqlca) Another topic is that, CI already catched this [1] (and other not related things in cpluspluscheck). However, it seems that since we run headerscheck and cpluspluscheck with the '-fmax-errors=10' option; CI didn't fail. It would be good to remove '-fmax-errors=10', though. [1] https://cirrus-ci.com/task/5166361724321792?logs=headers_headerscheck#L0 -- Regards, Nazir Bilal Yavuz Microsoft
Nazir Bilal Yavuz <byavuz81@gmail.com> writes:
> I got this warning while running headerscheck after this commit:
> ~/Desktop/projects/postgres/src/interfaces/ecpg/ecpglib/ecpglib_extern.h:221:40:
> warning: function declaration isn’t a prototype [-Wstrict-prototypes]
> 221 | void ecpg_init_sqlca(struct sqlca_t *sqlca)
Yeah, I see that too. I believe the problem is that headerscheck
doesn't cause POSTGRES_ECPG_INTERNAL to become defined, so that
what the compiler is seeing is (from sqlca.h):
#ifndef POSTGRES_ECPG_INTERNAL
#define sqlca (*ECPGget_sqlca())
#endif
and then
void ecpg_init_sqlca(struct sqlca_t *sqlca);
Kinda surprising that that's not a syntax error. We could plausibly
fix this either by
(1) renaming ecpg_init_sqlca's parameter to something else;
(2) ensuring that POSTGRES_ECPG_INTERNAL is defined. I'd be inclined
to make ecpglib_extern.h do that rather than expecting headerscheck
to know about it.
Neither of these options are beautiful, but perhaps #1 is slightly
less ugly. Any preferences?
regards, tom lane
Hi, On 2026-03-18 13:03:03 -0400, Tom Lane wrote: > Nazir Bilal Yavuz <byavuz81@gmail.com> writes: > > I got this warning while running headerscheck after this commit: > > > ~/Desktop/projects/postgres/src/interfaces/ecpg/ecpglib/ecpglib_extern.h:221:40: > > warning: function declaration isn’t a prototype [-Wstrict-prototypes] > > 221 | void ecpg_init_sqlca(struct sqlca_t *sqlca) > > Yeah, I see that too. I believe the problem is that headerscheck > doesn't cause POSTGRES_ECPG_INTERNAL to become defined, so that > what the compiler is seeing is (from sqlca.h): > > #ifndef POSTGRES_ECPG_INTERNAL > #define sqlca (*ECPGget_sqlca()) > #endif > > and then > > void ecpg_init_sqlca(struct sqlca_t *sqlca); > > Kinda surprising that that's not a syntax error. Indeed. I don't even really understand what that POSTGRES_ECPG_INTERNAL business is about. Is that really just so we can use a local "sqlca" variable in some of our own ecpglib code while code using ecpg can't do that? Please tell me it ain't so. > We could plausibly fix this either by > > (1) renaming ecpg_init_sqlca's parameter to something else; > > (2) ensuring that POSTGRES_ECPG_INTERNAL is defined. I'd be inclined > to make ecpglib_extern.h do that rather than expecting headerscheck > to know about it. > Neither of these options are beautiful, but perhaps #1 is slightly > less ugly. Any preferences? 1) seems to be the preferrable approach. It's what code using ecpg already has to do, right? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2026-03-18 13:03:03 -0400, Tom Lane wrote:
>> Yeah, I see that too. I believe the problem is that headerscheck
>> doesn't cause POSTGRES_ECPG_INTERNAL to become defined, so that
>> what the compiler is seeing is (from sqlca.h):
>>
>> #ifndef POSTGRES_ECPG_INTERNAL
>> #define sqlca (*ECPGget_sqlca())
>> #endif
> I don't even really understand what that POSTGRES_ECPG_INTERNAL business is
> about. Is that really just so we can use a local "sqlca" variable in some of
> our own ecpglib code while code using ecpg can't do that? Please tell me it
> ain't so.
Looks like it's so --- AFAICS, POSTGRES_ECPG_INTERNAL isn't tested
anywhere except in this fragment in sqlca.h.
>> We could plausibly fix this either by
>>
>> (1) renaming ecpg_init_sqlca's parameter to something else;
>>
>> (2) ensuring that POSTGRES_ECPG_INTERNAL is defined. I'd be inclined
>> to make ecpglib_extern.h do that rather than expecting headerscheck
>> to know about it.
>> Neither of these options are beautiful, but perhaps #1 is slightly
>> less ugly. Any preferences?
> 1) seems to be the preferrable approach. It's what code using ecpg already has
> to do, right?
I think that from the point of view of ecpg client applications,
"sqlca" is supposed to be a library-defined global variable
(and this macro is about making it thread-local).
They'd not really have use for local variables named that.
regards, tom lane
Hi,
On Wed, Mar 18, 2026 at 01:03:03PM -0400, Tom Lane wrote:
> Nazir Bilal Yavuz <byavuz81@gmail.com> writes:
> > I got this warning while running headerscheck after this commit:
Thanks for the report!
> > ~/Desktop/projects/postgres/src/interfaces/ecpg/ecpglib/ecpglib_extern.h:221:40:
> > warning: function declaration isn’t a prototype [-Wstrict-prototypes]
> > 221 | void ecpg_init_sqlca(struct sqlca_t *sqlca)
>
> Yeah, I see that too. I believe the problem is that headerscheck
> doesn't cause POSTGRES_ECPG_INTERNAL to become defined, so that
> what the compiler is seeing is (from sqlca.h):
>
> #ifndef POSTGRES_ECPG_INTERNAL
> #define sqlca (*ECPGget_sqlca())
> #endif
>
> and then
>
> void ecpg_init_sqlca(struct sqlca_t *sqlca);
Oh right, confirmed with:
"
$ gcc -E -I src/interfaces/ecpg/include -I src/include -I src/interfaces/libpq \
-include src/include/postgres_fe.h src/interfaces/ecpg/ecpglib/ecpglib_extern.h \
| grep ECPGget_sqlca
$ struct sqlca_t *ECPGget_sqlca(void);
$ void ecpg_init_sqlca(struct sqlca_t *(*ECPGget_sqlca()));
"
> Kinda surprising that that's not a syntax error.
Yeah...
> We could plausibly fix this either by
>
> (1) renaming ecpg_init_sqlca's parameter to something else;
>
> (2) ensuring that POSTGRES_ECPG_INTERNAL is defined. I'd be inclined
> to make ecpglib_extern.h do that rather than expecting headerscheck
> to know about it.
>
> Neither of these options are beautiful, but perhaps #1 is slightly
> less ugly. Any preferences?
I'd vote for #1 too, done in the attached.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Вложения
Bertrand Drouvot <bertranddrouvot.pg@gmail.com> writes:
> On Wed, Mar 18, 2026 at 01:03:03PM -0400, Tom Lane wrote:
>> We could plausibly fix this either by
>>
>> (1) renaming ecpg_init_sqlca's parameter to something else;
>>
>> (2) ensuring that POSTGRES_ECPG_INTERNAL is defined. I'd be inclined
>> to make ecpglib_extern.h do that rather than expecting headerscheck
>> to know about it.
>>
>> Neither of these options are beautiful, but perhaps #1 is slightly
>> less ugly. Any preferences?
> I'd vote for #1 too, done in the attached.
Agreed and pushed.
regards, tom lane
On 18.03.26 14:32, Peter Eisentraut wrote: > On 16.03.26 10:55, Peter Eisentraut wrote: >> On 09.03.26 17:39, Bertrand Drouvot wrote: >>> 0001: Prevent -Wstrict-prototypes and -Wold-style-definition warnings >>> >>> It fixes the remaining warnings that those new flags would generate. >> >> I have committed this one. I'll look at the rest next. > > Also committed. I have a couple of follow-up patches that I had developed while playing with this. There is a warning option for MSVC that appears to have a very similar effect to the ones we added here, so I propose we add that one as well. Additionally, there is an option for MSVC to disable warnings in system headers, similar to the default behavior of GCC. This would be required here because some system header files have non-strict prototypes. Additionally, I propose to add -Wold-style-declaration, which is completely unrelated to -Wold-style-definition, but it has popped up a few times via the buildfarm (grep for it in git log), so I think we might as well add it so that everyone sees it. Finally, I think we can remove the option -Wendif-labels, which doesn't do anything anymore that isn't the default. (It was only not the default before gcc 4.0.)
Вложения
Peter Eisentraut <peter@eisentraut.org> writes:
> I have a couple of follow-up patches that I had developed while playing
> with this.
I'm not in a position to evaluate the MSVC-specific changes, but
+1 for the other two.
regards, tom lane
Hi, On Mon, Mar 23, 2026 at 04:13:20PM +0100, Peter Eisentraut wrote: > On 18.03.26 14:32, Peter Eisentraut wrote: > > On 16.03.26 10:55, Peter Eisentraut wrote: > > > On 09.03.26 17:39, Bertrand Drouvot wrote: > > > > 0001: Prevent -Wstrict-prototypes and -Wold-style-definition warnings > > > > > > > > It fixes the remaining warnings that those new flags would generate. > > > > > > I have committed this one. I'll look at the rest next. > > > > Also committed. > > I have a couple of follow-up patches that I had developed while playing with > this. Thanks! > There is a warning option for MSVC that appears to have a very similar > effect to the ones we added here, so I propose we add that one as well. > > Additionally, there is an option for MSVC to disable warnings in system > headers, similar to the default behavior of GCC. This would be required > here because some system header files have non-strict prototypes. Some comments: 0001: + '/external:anglebrackets', + '/external:W0', The doc [1], states: " The /external compiler options are available starting in Visual Studio 2017 version 15.6. In versions of Visual Studio before Visual Studio 2019 version 16.10, the /external options require you also set the /experimental:external compiler option. " We currently require MSVC 2019, but what if one is using a version < 16.10? 0003: " This has been the default since gcc 4.0. (Introduced in 3.3, so it was only available but turned off for a relatively short time.) " It looks like it was default to on since its introduction in 909de5da19 ([2]), means since 3.3. In 90689ae11db ([3]), added in 4.0, the documentation has been updated to mention it (but I think that it was already on by default since 3.3). 0004: --- a/meson.build +++ b/meson.build @@ -2199,6 +2199,7 @@ unroll_loops_cflags = cc.get_supported_arguments(['-funroll-loops']) common_warning_flags = [ '-Wmissing-prototypes', + '-Wold-style-declaration', Nit, what about adding it with (as the comment is also accurate for the new one)? " # These are C-only flags, supported in all C11-capable GCC/Clang versions. cflags_warn += cc.get_supported_arguments(['-Wstrict-prototypes', '-Wold-style-definition']) " [1]: https://learn.microsoft.com/en-us/cpp/build/reference/external-external-headers-diagnostics?view=msvc-170 [2]: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=909de5da192 [3]: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=90689ae11db Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On 24.03.26 08:16, Bertrand Drouvot wrote: > 0001: > > + '/external:anglebrackets', > + '/external:W0', > > The doc [1], states: > > " > The /external compiler options are available starting in Visual Studio 2017 version 15.6. > In versions of Visual Studio before Visual Studio 2019 version 16.10, the /external > options require you also set the /experimental:external compiler option. > " > > We currently require MSVC 2019, but what if one is using a version < 16.10? Per discussion in [0] we effectively require 16.11, so I think this should be ok. [0]: https://www.postgresql.org/message-id/04ab76a3-186c-4a37-8076-e6882ebf9d43%40eisentraut.org
Hi, On Tue, Mar 24, 2026 at 10:36:43PM +0100, Peter Eisentraut wrote: > On 24.03.26 08:16, Bertrand Drouvot wrote: > > 0001: > > > > + '/external:anglebrackets', > > + '/external:W0', > > > > The doc [1], states: > > > > " > > The /external compiler options are available starting in Visual Studio 2017 version 15.6. > > In versions of Visual Studio before Visual Studio 2019 version 16.10, the /external > > options require you also set the /experimental:external compiler option. > > " > > > > We currently require MSVC 2019, but what if one is using a version < 16.10? > > Per discussion in [0] we effectively require 16.11, so I think this should > be ok. Agree, thanks for the link! Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com