Horton

Thread

Commits

  1. Fix C23 compiler warning

  2. Rename C23 keyword

  1. Fix C23 compiler warning

    Peter Eisentraut <peter@eisentraut.org> — 2024-10-20T07:07:16Z

    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.

  2. Re: Fix C23 compiler warning

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-20T15:56:55Z

    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

  3. Re: Fix C23 compiler warning

    Peter Eisentraut <peter@eisentraut.org> — 2024-10-22T06:35:14Z

    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.

  4. Re: Fix C23 compiler warning

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-22T06:41:16Z

    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

  5. Re: Fix C23 compiler warning

    Peter Eisentraut <peter@eisentraut.org> — 2024-10-22T08:32:41Z

    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.

  6. Re: Fix C23 compiler warning

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-10-25T15:17:35Z

    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)

  7. Re: Fix C23 compiler warning

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-25T15:48:29Z

    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

  8. Re: Fix C23 compiler warning

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-10-26T14:16:27Z

    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/

  9. Re: Fix C23 compiler warning

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-26T21:18:35Z

    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

  10. Re: Fix C23 compiler warning

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-10-28T12:19:48Z

    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

  11. Re: Fix C23 compiler warning

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-28T14:14:12Z

    =?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