On Tue, Oct 22, 2019 at 3:42 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > Review for 0003-Ordering-of-header-files-remaining-dir-oct21 > ----------------------------------------------------------------------------------------- > 1. > --- a/src/bin/pg_basebackup/pg_recvlogical.c > +++ b/src/bin/pg_basebackup/pg_recvlogical.c > @@ -19,18 +19,16 @@ > #include <sys/select.h> > #endif > > -/* local > includes */ > -#include "streamutil.h" > - > #include "access/xlog_internal.h" > -#include "common/file_perm.h" > #include "common/fe_memutils.h" > +#include > "common/file_perm.h" > #include "common/logging.h" > #include "getopt_long.h" > #include "libpq-fe.h" > #include "libpq/pqsignal.h" > #include "pqexpbuffer.h" > > +#include "streamutil.h" > > Extra space before streamutil.h include. > Fixed. > 2. > --- a/src/interfaces/libpq/fe-connect.c > +++ b/src/interfaces/libpq/fe-connect.c > @@ -21,11 +21,6 @@ > #include <time.h> > #include <unistd.h> > > -#include > "libpq-fe.h" > -#include "libpq-int.h" > -#include "fe-auth.h" > -#include "pg_config_paths.h" > - > #ifdef WIN32 > #include "win32.h" > #ifdef _WIN32_IE > @@ -74,10 > +69,13 @@ static int ldapServiceLookup(const char *purl, > PQconninfoOption *options, > #include "common/link-canary.h" > #include "common/scram- > common.h" > #include "common/string.h" > +#include "fe-auth.h" > +#include "libpq-fe.h" > +#include "libpq-int.h" > #include "mb/pg_wchar.h" > +#include > "pg_config_paths.h" > > After this change, the Windows build is failing for me. You forgot to > move the below code: > #ifdef USE_LDAP > #ifdef WIN32 > #include <winldap.h> > #else > /* OpenLDAP deprecates RFC 1823, but we want standard conformance */ > #define LDAP_DEPRECATED 1 > #include <ldap.h> > typedef struct timeval LDAP_TIMEVAL; > #endif > static int ldapServiceLookup(const char *purl, PQconninfoOption *options, > PQExpBuffer errorMessage); > #endif > > All this needs to be moved after all the includes. > Fixed. I don't have windows environment, let me know if you still face some issue. > 3. > /* ScanKeywordList lookup data for ECPG keywords */ > #include "ecpg_kwlist_d.h" > +#include "preproc_extern.h" > +#include "preproc.h" > > I think preproc.h include should be before preproc_extern.h due to the > reason mentioned earlier. > For this file the earlier order was also like that. As per the ordering preproc_extern.h should be before prepr.h. But the preproc.h has some dependency on preproc_extern.h. Not made any changes for this. Same is the case in c_keywords.c also. > 4. > --- a/src/test/modules/worker_spi/worker_spi.c > +++ b/src/test/modules/worker_spi/worker_spi.c > @@ -22,24 +22,22 @@ > */ > #include "postgres.h" > > -/* These are always necessary for a bgworker */ > +/* these headers are used by this particular worker's code */ > +#include "access/xact.h" > +#include "executor/spi.h" > +#include "fmgr.h" > +#include "lib/stringinfo.h" > #include "miscadmin.h" > +#include "pgstat.h" > #include "postmaster/bgworker.h" > #include "storage/ipc.h" > #include "storage/latch.h" > #include "storage/lwlock.h" > #include "storage/proc.h" > #include "storage/shmem.h" > - > -/* these headers are used by this particular worker's code */ > -#include "access/xact.h" > -#include "executor/spi.h" > > I am skeptical of this change as it is very clearly written in > comments the reason why header includes are separated. Fixed. Have reverted this change. Attached patch has the updated changes. > -- > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com
0002-Ordering-of-header-files-remaining-dir-oct22.patch
Description: Binary data
