Обсуждение: typedef struct LogicalDecodingContext

Hi,

During a recent code review, I noticed a lot of 'struct
LogicalDecodingContext' usage.

There are many function prototypes where the params are (for no
apparent reason to me) a mixture of structs and typedef structs.

AFAICT just by pre-declaring the typedef struct
LogicalDecodingContext, all of those 'struct LogicalDecodingContext'
can be culled, resulting in cleaner and more consistent function
signatures.

The PG Docs were similarly modified.

PSA patch for this.  It passes make check-world.

(I recognize this is potentially the tip of an iceberg. If this patch
is deemed OK, I can hunt down similar underuse of typedefs for other
structs)

Thoughts?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Peter Smith <smithpb2250@gmail.com> writes:
> AFAICT just by pre-declaring the typedef struct
> LogicalDecodingContext, all of those 'struct LogicalDecodingContext'
> can be culled, resulting in cleaner and more consistent function
> signatures.

Sadly, this is almost certainly going to cause bitching on the part of
some compilers, because depending on the order of header inclusions
they are going to see multiple typedefs for the same name.  Redundant
"struct foo" declarations are portable C, but redundant "typedef foo"
not so much.

I also wonder if this passes headerscheck and cpluspluscheck.

            regards, tom lane

On Thu, Mar 2, 2023 at 10:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Smith <smithpb2250@gmail.com> writes:
> > AFAICT just by pre-declaring the typedef struct
> > LogicalDecodingContext, all of those 'struct LogicalDecodingContext'
> > can be culled, resulting in cleaner and more consistent function
> > signatures.
>
> Sadly, this is almost certainly going to cause bitching on the part of
> some compilers, because depending on the order of header inclusions
> they are going to see multiple typedefs for the same name.  Redundant
> "struct foo" declarations are portable C, but redundant "typedef foo"
> not so much.
>

Bah, I should not have used that tip-of-an-iceberg metaphor; it turns
out I was actually standing on the ship...

So does your reply mean there is no way really to be sure if such
changes are OK or not, other than to push them and then revert them
if/when one of the BF animals complains? If that is the case, then
it's not worth the hassle to pursue this any further.

> I also wonder if this passes headerscheck and cpluspluscheck.

Thanks for pointing me to those - I didn't know about them.

Aside: Is there missing documentation for those targets here:
https://www.postgresql.org/docs/devel/regress.html

~

FWIW, both those tests passed OK. What does "pass" even mean -- does
it confirm this patch doesn't suffer the multiple typedef problem you
anticipated after all?

[postgres@CentOS7-x64 oss_postgres_misc]$ make headerscheck
make -C ./src/backend generated-headers
make[1]: Entering directory `/home/postgres/oss_postgres_misc/src/backend'
make -C catalog distprep generated-header-symlinks
make[2]: Entering directory
`/home/postgres/oss_postgres_misc/src/backend/catalog'
make[2]: Nothing to be done for `distprep'.
make[2]: Nothing to be done for `generated-header-symlinks'.
make[2]: Leaving directory
`/home/postgres/oss_postgres_misc/src/backend/catalog'
make -C nodes distprep generated-header-symlinks
make[2]: Entering directory `/home/postgres/oss_postgres_misc/src/backend/nodes'
make[2]: Nothing to be done for `distprep'.
make[2]: Nothing to be done for `generated-header-symlinks'.
make[2]: Leaving directory `/home/postgres/oss_postgres_misc/src/backend/nodes'
make -C utils distprep generated-header-symlinks
make[2]: Entering directory `/home/postgres/oss_postgres_misc/src/backend/utils'
make[2]: Nothing to be done for `distprep'.
make[2]: Nothing to be done for `generated-header-symlinks'.
make[2]: Leaving directory `/home/postgres/oss_postgres_misc/src/backend/utils'
make[1]: Leaving directory `/home/postgres/oss_postgres_misc/src/backend'
./src/tools/pginclude/headerscheck . /home/postgres/oss_postgres_misc
[postgres@CentOS7-x64 oss_postgres_misc]$


[postgres@CentOS7-x64 oss_postgres_misc]$ make cpluspluscheck
make -C ./src/backend generated-headers
make[1]: Entering directory `/home/postgres/oss_postgres_misc/src/backend'
make -C catalog distprep generated-header-symlinks
make[2]: Entering directory
`/home/postgres/oss_postgres_misc/src/backend/catalog'
make[2]: Nothing to be done for `distprep'.
make[2]: Nothing to be done for `generated-header-symlinks'.
make[2]: Leaving directory
`/home/postgres/oss_postgres_misc/src/backend/catalog'
make -C nodes distprep generated-header-symlinks
make[2]: Entering directory `/home/postgres/oss_postgres_misc/src/backend/nodes'
make[2]: Nothing to be done for `distprep'.
make[2]: Nothing to be done for `generated-header-symlinks'.
make[2]: Leaving directory `/home/postgres/oss_postgres_misc/src/backend/nodes'
make -C utils distprep generated-header-symlinks
make[2]: Entering directory `/home/postgres/oss_postgres_misc/src/backend/utils'
make[2]: Nothing to be done for `distprep'.
make[2]: Nothing to be done for `generated-header-symlinks'.
make[2]: Leaving directory `/home/postgres/oss_postgres_misc/src/backend/utils'
make[1]: Leaving directory `/home/postgres/oss_postgres_misc/src/backend'
./src/tools/pginclude/cpluspluscheck . /home/postgres/oss_postgres_misc
[postgres@CentOS7-x64 oss_postgres_misc]$

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Peter Smith <smithpb2250@gmail.com> writes:
> On Thu, Mar 2, 2023 at 10:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Sadly, this is almost certainly going to cause bitching on the part of
>> some compilers, because depending on the order of header inclusions
>> they are going to see multiple typedefs for the same name.  Redundant
>> "struct foo" declarations are portable C, but redundant "typedef foo"
>> not so much.

> So does your reply mean there is no way really to be sure if such
> changes are OK or not, other than to push them and then revert them
> if/when one of the BF animals complains?

We know which compilers don't like that, I believe, but you'd have
to dig in the commit log or mail archives to find out.

[ ... pokes around ... ]  The commit log entries I could find about
this suggest that (at least) older gcc versions complain.  Maybe
those are all gone from the buildfarm now, but I wouldn't bet on it.
We were fixing this sort of thing as recently as aa3ac6453.

>> I also wonder if this passes headerscheck and cpluspluscheck.

> FWIW, both those tests passed OK. What does "pass" even mean -- does
> it confirm this patch doesn't suffer the multiple typedef problem you
> anticipated after all?

No, those have nothing to do with duplicate typedefs.  headerscheck is
about whether anything is dependent on inclusion order, which I wondered
about for this patch.  cpluspluscheck is about whether C++ compilers will
spit up on any of our headers (due to, eg, identifiers that are C++
keywords); we try to keep them clean for the benefit of people who write
extensions in C++.  I wouldn't have expected cpluspluscheck to show
anything new with this patch, but people tend to always run these tools
together.

            regards, tom lane

I wrote:
> Peter Smith <smithpb2250@gmail.com> writes:
>> On Thu, Mar 2, 2023 at 10:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Sadly, this is almost certainly going to cause bitching on the part of
>>> some compilers, because depending on the order of header inclusions
>>> they are going to see multiple typedefs for the same name.

>> So does your reply mean there is no way really to be sure if such
>> changes are OK or not, other than to push them and then revert them
>> if/when one of the BF animals complains?

> We know which compilers don't like that, I believe, but you'd have
> to dig in the commit log or mail archives to find out.

I looked into the C standard to see what I could find about this.
C99 specifically describes the use of "struct foo" to forward-declare
a struct type whose meaning will be provided later.  It also says

       [#8] If a type specifier of the form
               struct-or-union identifier
       or
               enum identifier
       occurs  other  than as part of one of the above forms, and a
       declaration of the identifier as a tag is visible,  then  it
       specifies  the same type as that other declaration, and does
       not redeclare the tag.

which appears to me to specifically authorize the appearance of
multiple forward declarations.  On the other hand, no such wording
appears for typedefs; they're just plain identifiers with the same
scope rules as other identifiers.  Maybe later versions of the C
spec clarify this, but I think duplicate typedefs are pretty
clearly not OK per C99.  Perhaps with sufficiently tight warning
or language-version options, you could get modern gcc or clang to
complain about it.

            regards, tom lane

I wrote:
> Maybe later versions of the C
> spec clarify this, but I think duplicate typedefs are pretty
> clearly not OK per C99.

Further research shows that C11 allows this, but it's definitely
not okay in C99, which is still our reference standard.

> Perhaps with sufficiently tight warning
> or language-version options, you could get modern gcc or clang to
> complain about it.

clang seems to do so as soon as you restrict it to C99:

$ cat dup.c
typedef int foo;
typedef int foo;
$ clang -c -std=gnu99 dup.c
dup.c:2:13: warning: redefinition of typedef 'foo' is a C11 feature [-Wtypedef-redefinition]
typedef int foo;
            ^
dup.c:1:13: note: previous definition is here
typedef int foo;
            ^
1 warning generated.

I couldn't get gcc to issue a similar warning without resorting
to -Wpedantic, which of course whines about a ton of other stuff.

I'm a little inclined to see if I can turn on -std=gnu99 on my
clang-based buildfarm animals.  I use that with gcc for my
normal development activities, but now that I see that clang
catches some things gcc doesn't ...

            regards, tom lane

On Thu, Mar 2, 2023 at 12:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Peter Smith <smithpb2250@gmail.com> writes:
> >> On Thu, Mar 2, 2023 at 10:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> Sadly, this is almost certainly going to cause bitching on the part of
> >>> some compilers, because depending on the order of header inclusions
> >>> they are going to see multiple typedefs for the same name.
>
> >> So does your reply mean there is no way really to be sure if such
> >> changes are OK or not, other than to push them and then revert them
> >> if/when one of the BF animals complains?
>
> > We know which compilers don't like that, I believe, but you'd have
> > to dig in the commit log or mail archives to find out.
>
> I looked into the C standard to see what I could find about this.
> C99 specifically describes the use of "struct foo" to forward-declare
> a struct type whose meaning will be provided later.  It also says
>
>        [#8] If a type specifier of the form
>                struct-or-union identifier
>        or
>                enum identifier
>        occurs  other  than as part of one of the above forms, and a
>        declaration of the identifier as a tag is visible,  then  it
>        specifies  the same type as that other declaration, and does
>        not redeclare the tag.
>
> which appears to me to specifically authorize the appearance of
> multiple forward declarations.  On the other hand, no such wording
> appears for typedefs; they're just plain identifiers with the same
> scope rules as other identifiers.  Maybe later versions of the C
> spec clarify this, but I think duplicate typedefs are pretty
> clearly not OK per C99.  Perhaps with sufficiently tight warning
> or language-version options, you could get modern gcc or clang to
> complain about it.

I was reading this post [1], and more specifically, this specification
note [2] which seems to explain things

Apparently, not all C99 compilers can be assumed to work using the
strict C99 rules. So I will abandon this idea.

Thanks for your replies.

------
[1]
https://stackoverflow.com/questions/26240370/why-are-typedef-identifiers-allowed-to-be-declared-multiple-times/26240595#26240595
[2] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1360.htm

Kind Regards,
Peter Smith.
Fujitsu Australia

Peter Smith <smithpb2250@gmail.com> writes:
> Apparently, not all C99 compilers can be assumed to work using the
> strict C99 rules.

While googling this issue I came across a statement that clang currently
defaults to C17 rules.  Even relatively old compilers might default to
C11.  But considering how long we held on to C89, I doubt we'll want
to move the project minimum to C11 for some years yet.

> So I will abandon this idea.

There might still be room to do something here, just not quite
that way.  Maybe some actual header refactoring is called for?

            regards, tom lane

I wrote:
> I'm a little inclined to see if I can turn on -std=gnu99 on my
> clang-based buildfarm animals.  I use that with gcc for my
> normal development activities, but now that I see that clang
> catches some things gcc doesn't ...

FTR: done on sifaka and longfin.

            regards, tom lane

On 02.03.23 04:00, Tom Lane wrote:
> I wrote:
>> I'm a little inclined to see if I can turn on -std=gnu99 on my
>> clang-based buildfarm animals.  I use that with gcc for my
>> normal development activities, but now that I see that clang
>> catches some things gcc doesn't ...
> 
> FTR: done on sifaka and longfin.

mylodon already does something similar.

On 02.03.23 03:46, Tom Lane wrote:
> Peter Smith <smithpb2250@gmail.com> writes:
>> Apparently, not all C99 compilers can be assumed to work using the
>> strict C99 rules.
> 
> While googling this issue I came across a statement that clang currently
> defaults to C17 rules.  Even relatively old compilers might default to
> C11.  But considering how long we held on to C89, I doubt we'll want
> to move the project minimum to C11 for some years yet.

We need to wait until we de-support Visual Studio older then 2019. 
(Current minimum is 2015 (changed from 2013 for PG16).)