Re: Ordering of header file inclusion

On Tue, Oct 22, 2019 at 3:42 PM Amit Kapila <amit.kapila16@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