Thread: Re: further #include cleanup (IWYU)
On 2024-Oct-20, Peter Eisentraut wrote: > diff --git a/src/bin/pg_dump/pg_backup_utils.c b/src/bin/pg_dump/pg_backup_utils.c > index a0045cf5e58..80715979a1a 100644 > --- a/src/bin/pg_dump/pg_backup_utils.c > +++ b/src/bin/pg_dump/pg_backup_utils.c > @@ -13,7 +13,9 @@ > */ > #include "postgres_fe.h" > > +#ifdef WIN32 > #include "parallel.h" > +#endif > #include "pg_backup_utils.h" This seems quite weird and I think it's just because exit_nicely() wants to do _endthreadex(). Maybe it'd be nicer to add a WIN32-specific on_exit_nicely_list() callback that does that in parallel.c, and do away with the inclusion of parallel.h in pg_backup_utils.c entirely? > diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c > index a09247fae47..78e91f6e2dc 100644 > --- a/src/bin/pg_dump/parallel.c > +++ b/src/bin/pg_dump/parallel.c > @@ -63,7 +63,9 @@ > #include "fe_utils/string_utils.h" > #include "parallel.h" > #include "pg_backup_utils.h" > +#ifdef WIN32 > #include "port/pg_bswap.h" > +#endif This looks really strange, but then parallel.c seems to have embedded its own portability layer within itself. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "I'm impressed how quickly you are fixing this obscure issue. I came from MS SQL and it would be hard for me to put into words how much of a better job you all are doing on [PostgreSQL]." Steve Midgley, http://archives.postgresql.org/pgsql-sql/2008-08/msg00000.php
On 20.10.24 11:53, Alvaro Herrera wrote: > On 2024-Oct-20, Peter Eisentraut wrote: >> diff --git a/src/bin/pg_dump/pg_backup_utils.c b/src/bin/pg_dump/pg_backup_utils.c >> index a0045cf5e58..80715979a1a 100644 >> --- a/src/bin/pg_dump/pg_backup_utils.c >> +++ b/src/bin/pg_dump/pg_backup_utils.c >> @@ -13,7 +13,9 @@ >> */ >> #include "postgres_fe.h" >> >> +#ifdef WIN32 >> #include "parallel.h" >> +#endif >> #include "pg_backup_utils.h" > > This seems quite weird and I think it's just because exit_nicely() wants > to do _endthreadex(). Maybe it'd be nicer to add a WIN32-specific > on_exit_nicely_list() callback that does that in parallel.c, and do away > with the inclusion of parallel.h in pg_backup_utils.c entirely? I was thinking the same thing. But maybe that should be a separate project. >> diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c >> index a09247fae47..78e91f6e2dc 100644 >> --- a/src/bin/pg_dump/parallel.c >> +++ b/src/bin/pg_dump/parallel.c >> @@ -63,7 +63,9 @@ >> #include "fe_utils/string_utils.h" >> #include "parallel.h" >> #include "pg_backup_utils.h" >> +#ifdef WIN32 >> #include "port/pg_bswap.h" >> +#endif > > This looks really strange, but then parallel.c seems to have embedded > its own portability layer within itself. The reason for this one is that pgpipe() uses pg_hton16() and pg_hton32(). We could use htons() and htonl() here instead. That would effectively revert that part of commit 0ba99c84e8c.
Seeing no further comments (or any easy alternatives), I have committed this last patch as is. On 28.10.24 10:45, Peter Eisentraut wrote: > On 20.10.24 11:53, Alvaro Herrera wrote: >> On 2024-Oct-20, Peter Eisentraut wrote: >>> diff --git a/src/bin/pg_dump/pg_backup_utils.c b/src/bin/pg_dump/ >>> pg_backup_utils.c >>> index a0045cf5e58..80715979a1a 100644 >>> --- a/src/bin/pg_dump/pg_backup_utils.c >>> +++ b/src/bin/pg_dump/pg_backup_utils.c >>> @@ -13,7 +13,9 @@ >>> */ >>> #include "postgres_fe.h" >>> +#ifdef WIN32 >>> #include "parallel.h" >>> +#endif >>> #include "pg_backup_utils.h" >> >> This seems quite weird and I think it's just because exit_nicely() wants >> to do _endthreadex(). Maybe it'd be nicer to add a WIN32-specific >> on_exit_nicely_list() callback that does that in parallel.c, and do away >> with the inclusion of parallel.h in pg_backup_utils.c entirely? > > I was thinking the same thing. But maybe that should be a separate > project. > >>> diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c >>> index a09247fae47..78e91f6e2dc 100644 >>> --- a/src/bin/pg_dump/parallel.c >>> +++ b/src/bin/pg_dump/parallel.c >>> @@ -63,7 +63,9 @@ >>> #include "fe_utils/string_utils.h" >>> #include "parallel.h" >>> #include "pg_backup_utils.h" >>> +#ifdef WIN32 >>> #include "port/pg_bswap.h" >>> +#endif >> >> This looks really strange, but then parallel.c seems to have embedded >> its own portability layer within itself. > > The reason for this one is that pgpipe() uses pg_hton16() and > pg_hton32(). We could use htons() and htonl() here instead. That would > effectively revert that part of commit 0ba99c84e8c. > >
On Wed, 6 Nov 2024 at 11:50, Peter Eisentraut <peter@eisentraut.org> wrote: > > Seeing no further comments (or any easy alternatives), I have committed > this last patch as is. I just noticed that the #define for MaxArraySize in utils/array.h uses MaxAllocSize without including the utils/memutils.h header. Is that on purpose and is the user expected to include both headers, or should utils/memutils.h be included in utils/array.h? Kind regards, Matthias van de Meent Neon (https://neon.tech)
On 27.01.25 15:10, Matthias van de Meent wrote: > On Wed, 6 Nov 2024 at 11:50, Peter Eisentraut <peter@eisentraut.org> wrote: >> >> Seeing no further comments (or any easy alternatives), I have committed >> this last patch as is. > > I just noticed that the #define for MaxArraySize in utils/array.h uses > MaxAllocSize without including the utils/memutils.h header. Is that on > purpose and is the user expected to include both headers, or should > utils/memutils.h be included in utils/array.h? I have found a number of cases like this, where a macro definition in a header uses a symbol that is defined in another header that is not included. This is not something that the IWYU tool set can track. In general, this kind of thing is probably best avoided, but it would probably require individual investigation of each case.