On Tue, Oct 22, 2019 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 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?
>
Fixed.
Order is based on ascii table. Upper case letters will come before
lower case letters.
> 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).
>
Order is based on ascii table.
Ascii value for "." is 46, Ascii value for "-" is 45.
Hence have placed like:
#include "px-crypt.h"
#include "px.h"
Not make any changes for this. If still required I can modify.
> 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.
>
The changes are done based on ascii table.
Ascii value of "_" is 95.
Ascii value of "a" is 97.
Hence _int.h is place before access/gist.h.
I have not made any changes for this. If still required I can modify.
> 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.
>
Fixed.
> 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.
>
Fixed.
Thanks for the comments.
Attached patch has the updated changes.
Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com