Re: Ordering of header file inclusion

On Tue, Oct 22, 2019 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Oct 21, 2019 at 11:04 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Mon, Oct 21, 2019 at 8:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > >
> > Thanks for the suggestions.
> > Updated patch contains the fix based on Tom Lane's Suggestion.
> > Let me know your thoughts for further revision if required.
> >

This patch series has broadly changed the code to organize the header
includes in alphabetic order.  It also makes sure that all files first
includes 'postgres.h'/'postgres_fe.h', system header includes and then
Postgres header includes.

It also has a change where it seems that for local header includes, we
have used '<>' whereas quotes ("") should have been used.  See,
ecpg/compatlib/informix.c.

I am planning to commit this as multiple commits (a. contrib modules,
b. non-backend changes and c. backend changes) as there is some risk
of buildfarm break.  From my side, I will ensure that everything is
passing on windows and centos.  Any objections to this plan?

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.

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.

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.

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.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com