Re: Ordering of header file inclusion

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.
>

Few comments on 0001-Ordering-of-header-files-in-contrib-dir-oct21.patch
-----------------------------------------------------------------------------------------------------------
1.
--- a/contrib/isn/isn.c
+++ b/contrib/isn/isn.c
@@ -15,9 +15,9 @@
 #include "postgres.h"

 #include "fmgr.h"
+#include "isn.h"
 #include "utils/builtins.h"

-#include "isn.h"
 #include "EAN13.h"
 #include "ISBN.h"
 #include "ISMN.h"

Why only "isn.h" is moved and not others?

2.
+++ b/contrib/pgcrypto/px-crypt.c
@@ -31,9 +31,8 @@

 #include "postgres.h"

-#include "px.h"
 #include "px-crypt.h"
-
+#include "px.h"

I think such ordering was fine.  Forex. see, hash.c (hash.h was
included first and then hash_xlog.h).

3.
+#include "_int.h"
 #include "access/gist.h"
 #include "access/stratnum.h"

-#include "_int.h"
-

Do we need to give preference to '_'?  Is it being done somewhere
else?  It is not that this is wrong, just that I am not sure about
this.

4.
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -8,6 +8,7 @@
 #include "access/htup_details.h"
 #include "catalog/pg_type.h"
 #include "funcapi.h"
+#include "hstore.h"
 #include "lib/stringinfo.h"
 #include "libpq/pqformat.h"
 #include "utils/builtins.h"
@@ -18,7 +19,6 @@
 #include "utils/memutils.h"
 #include "utils/typcache.h"

-#include "hstore.h"

 PG_MODULE_MAGIC;

This created an extra white line.

5.
While reviewing, I noticed that in contrib/intarray/_int_op.c, there
is an extra white line between postgres.h and its first include.  I
think we can make that as well consistent.

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