Обсуждение: 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).)