Thread: Re: Fix C23 compiler warning
Peter Eisentraut <peter@eisentraut.org> writes:
> This no longer works because in C23, because an empty argument list is
> now equivalent to (void), rather than an indeterminate one as before.
> And so this results in an incompatible function pointer type and
> compiler warnings. (gcc and clang agree on this.)
> I think we can fix this easily with a few struct forward declarations,
> preserving the goal of not including extra header files, like this:
Do the struct declarations themselves need comments? Other
places do this like
struct PlannerInfo; /* avoid including pathnodes.h here */
LGTM other than that nit.
regards, tom lane
On 20.10.24 17:56, Tom Lane wrote: > Peter Eisentraut <peter@eisentraut.org> writes: >> This no longer works because in C23, because an empty argument list is >> now equivalent to (void), rather than an indeterminate one as before. >> And so this results in an incompatible function pointer type and >> compiler warnings. (gcc and clang agree on this.) > >> I think we can fix this easily with a few struct forward declarations, >> preserving the goal of not including extra header files, like this: > > Do the struct declarations themselves need comments? Other > places do this like > > struct PlannerInfo; /* avoid including pathnodes.h here */ > > LGTM other than that nit. Committed with that change. Thanks.
Peter Eisentraut <peter@eisentraut.org> writes:
> Committed with that change. Thanks.
Should we back-patch this? (And also a67a49648d9?) It's
not hard to imagine people wanting to compile our stable
branches with C23 compilers. I might leave out v12, which
is just days away from EOL, but this seems like a reasonable
change for all the later branches.
regards, tom lane
On 22.10.24 08:41, Tom Lane wrote: > Peter Eisentraut <peter@eisentraut.org> writes: >> Committed with that change. Thanks. > > Should we back-patch this? (And also a67a49648d9?) It's > not hard to imagine people wanting to compile our stable > branches with C23 compilers. I might leave out v12, which > is just days away from EOL, but this seems like a reasonable > change for all the later branches. One thing I didn't realize until today is that currently C23 compilations only work with meson. The autoconf version we are using doesn't support it, and the configure results it produces are somehow faulty and then you get a bunch of compilation errors. So if we wanted to make this a supported thing, it looks like we would need to use at least autoconf 2.72. So this then ties into further questions, like the future of autoconf support. Also, I think the compilers themselves are still finalizing their C23 support. (gcc-14 doesn't use the correct __STDC_VERSION__ version yet.) So I'm content to wait a little bit and see if there are more adjustments needed in the future. Maybe when gcc-15 comes out we'll have a more solid baseline.
On 2024-Oct-22, Peter Eisentraut wrote: > One thing I didn't realize until today is that currently C23 compilations > only work with meson. The autoconf version we are using doesn't support it, > and the configure results it produces are somehow faulty and then you get a > bunch of compilation errors. So if we wanted to make this a supported > thing, it looks like we would need to use at least autoconf 2.72. Oh wow. Should we at least update our autoconf requirement to 2.72 in master? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Digital and video cameras have this adjustment and film cameras don't for the same reason dogs and cats lick themselves: because they can." (Ken Rockwell)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2024-Oct-22, Peter Eisentraut wrote:
>> One thing I didn't realize until today is that currently C23 compilations
>> only work with meson. The autoconf version we are using doesn't support it,
>> and the configure results it produces are somehow faulty and then you get a
>> bunch of compilation errors. So if we wanted to make this a supported
>> thing, it looks like we would need to use at least autoconf 2.72.
> Oh wow. Should we at least update our autoconf requirement to 2.72 in
> master?
As I recall, we looked at adopting it some years ago and decided it
was too much churn for the value (seeing that the long-term plan is
to go to meson only). Maybe C23 is a reason to rethink, but from
what I recall of that, it won't be a painless update.
regards, tom lane
On 2024-Oct-25, Tom Lane wrote: > As I recall, we looked at adopting it some years ago and decided it > was too much churn for the value (seeing that the long-term plan is > to go to meson only). Maybe C23 is a reason to rethink, but from > what I recall of that, it won't be a painless update. Ah, yeah. That was 2.71 actually: https://postgr.es/m/3838336.1657985206@sss.pgh.pa.us 1.72 seems to have been released with some fixes from that one. Per that thread, the related problem you noticed was with m4, and apparently it was because macOS ships a version from 2006 (1.4.7). Here on Debian bookworm I have m4 1.4.19; maybe macOS has updated its copy now? -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > Ah, yeah. That was 2.71 actually: > https://postgr.es/m/3838336.1657985206@sss.pgh.pa.us > 1.72 seems to have been released with some fixes from that one. Per > that thread, the related problem you noticed was with m4, and apparently > it was because macOS ships a version from 2006 (1.4.7). Here on Debian > bookworm I have m4 1.4.19; maybe macOS has updated its copy now? macOS hasn't gotten better: $ which m4 /usr/bin/m4 $ m4 --version GNU M4 1.4.6 Copyright (C) 2006 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. It does seem like you-need-a-newer-m4 will be an issue for some folks, but given that you only need it to rebuild the configure script, maybe that will be a small enough set of people that we can cope. (In particular, the buildfarm wouldn't need updates.) On macOS, it's already pretty difficult to do useful development without any packages from MacPorts or Homebrew. MacPorts is shipping m4 1.4.19, and I'm sure Homebrew has something modern as well, so it's not like people would be forced to do their own builds on that platform. So maybe we should revive that idea, though I'd definitely target autoconf 2.72 not 2.71. regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: >> Ah, yeah. That was 2.71 actually: >> https://postgr.es/m/3838336.1657985206@sss.pgh.pa.us >> 1.72 seems to have been released with some fixes from that one. Per >> that thread, the related problem you noticed was with m4, and apparently >> it was because macOS ships a version from 2006 (1.4.7). Here on Debian >> bookworm I have m4 1.4.19; maybe macOS has updated its copy now? > > macOS hasn't gotten better: > > $ which m4 > /usr/bin/m4 > $ m4 --version > GNU M4 1.4.6 > Copyright (C) 2006 Free Software Foundation, Inc. > This is free software; see the source for copying conditions. There is NO > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. > > It does seem like you-need-a-newer-m4 will be an issue for some folks, > but given that you only need it to rebuild the configure script, maybe > that will be a small enough set of people that we can cope. (In > particular, the buildfarm wouldn't need updates.) On macOS, it's > already pretty difficult to do useful development without any packages > from MacPorts or Homebrew. MacPorts is shipping m4 1.4.19, and I'm > sure Homebrew has something modern as well, so it's not like people > would be forced to do their own builds on that platform. > > So maybe we should revive that idea, though I'd definitely target > autoconf 2.72 not 2.71. Just a data point: autoconf 2.72 came out under a year ago, so the most recent Debian Stable (12) and Ubuntu LTS (24.04) only have 2.71. If they stick to the the roughly-2-yearly cadence, Debian 13 will be out before PostgreSQL 18, but the next Ubuntu LTS release isn't until April 2026. They both have m4 1.4.19, though. > regards, tom lane - ilmari
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> So maybe we should revive that idea, though I'd definitely target
>> autoconf 2.72 not 2.71.
> Just a data point: autoconf 2.72 came out under a year ago, so the most
> recent Debian Stable (12) and Ubuntu LTS (24.04) only have 2.71.
I don't think we care, except to the extent that usage of 2.72 in
widely-used distros would increase confidence in it (which is far
from a trivial consideration). For many years, we've had a policy
that committers should use autoconf-built-from-GNU-sources rather
than distro packages. The distros tend to stick in local changes
that affect the output, but we need uniform output so that there's
not random churn in the committed version of the configure script.
Still, we're in wait-and-see mode about C23, so maybe wait-and-see
for awhile longer about autoconf 2.72 as well.
> They both have m4 1.4.19, though.
That's good news anyway. Per the older thread, building m4 from
source is no fun at all.
regards, tom lane