Fix C23 compiler warning
Commit a67a49648d9 fixed a compiler error under C23. After that, there
is one compiler warning left under C23. It has to do with this in
src/include/nodes/pathnodes.h:
struct IndexOptInfo
{
...
/* AM's cost estimator */
/* Rather than include amapi.h here, we declare amcostestimate like
this */
void (*amcostestimate) () pg_node_attr(read_write_ignore);
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:
struct IndexPath;
struct PlannerInfo;
struct IndexOptInfo
{
...
/* AM's cost estimator */
/* Rather than include amapi.h here, we declare amcostestimate like
this */
void (*amcostestimate) (struct PlannerInfo *, struct
IndexPath *, double, Cost *, Cost *, Selectivity *, double *, double *)
pg_node_attr(read_write_ignore);
That way the function pointer has the correct type. This works in older
versions of C as well.
Attachments:
0001-Fix-C23-compiler-warning.patchtext/plain; charset=UTF-8; name=0001-Fix-C23-compiler-warning.patchDownload
From 3be558bf4971904cd67cd9d80a5e2f52b66082b0 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sun, 20 Oct 2024 08:10:30 +0200
Subject: [PATCH] Fix C23 compiler warning
The approach of declaring a function pointer with an empty argument
list and hoping that the compiler will not complain about casting it
to another type no longer works with C23, because foo() is now
equivalent to foo(void).
We don't need to do this here. With a few struct forward declarations
we can supply a correct argument list without having to pull in
another header file.
(This is the only new warning with C23. Together with the previous
fix a67a49648d9, this makes the whole code compile cleanly under C23.)
---
src/include/nodes/pathnodes.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 07e2415398e..464c79c73a5 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -1107,6 +1107,9 @@ typedef struct IndexOptInfo IndexOptInfo;
#define HAVE_INDEXOPTINFO_TYPEDEF 1
#endif
+struct IndexPath;
+struct PlannerInfo;
+
struct IndexOptInfo
{
pg_node_attr(no_copy_equal, no_read, no_query_jumble)
@@ -1206,7 +1209,7 @@ struct IndexOptInfo
bool amcanmarkpos;
/* AM's cost estimator */
/* Rather than include amapi.h here, we declare amcostestimate like this */
- void (*amcostestimate) () pg_node_attr(read_write_ignore);
+ void (*amcostestimate) (struct PlannerInfo *, struct IndexPath *, double, Cost *, Cost *, Selectivity *, double *, double *) pg_node_attr(read_write_ignore);
};
/*
--
2.47.0
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 likestruct 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:
/messages/by-id/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:
/messages/by-id/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:
/messages/by-id/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