Re: Ordering of header file inclusion

On Mon, Oct 21, 2019 at 11:04 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, Oct 21, 2019 at 8:47 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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