On Thu, Oct 17, 2019 at 4:44 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Oct 15, 2019 at 10:57 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > Attached patch contains the fix based on the comments suggested.
> > I have added/deleted extra lines in certain places so that the
> > readability is better.
> >
>
> Hmm, I am not sure if that is better in all cases. It seems to be
> making the code look inconsistent at few places. See comments below:
>
> 1.
> diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
> index 4b2186b..45215ba 100644
> --- a/contrib/bloom/blinsert.c
> +++ b/contrib/bloom/blinsert.c
> @@ -15,6 +15,7 @@
> #include "access/genam.h"
> #include "access/generic_xlog.h"
> #include "access/tableam.h"
> +#include "bloom.h"
> #include "catalog/index.h"
> #include "miscadmin.h"
> #include "storage/bufmgr.h"
> @@ -23,7 +24,6 @@
> #include "utils/memutils.h"
> #include "utils/rel.h"
>
> -#include "bloom.h"
>
> PG_MODULE_MAGIC;
>
> diff --git a/contrib/bloom/blscan.c b/contrib/bloom/blscan.c
> index 49e364a..4b9a2b8 100644
> --- a/contrib/bloom/blscan.c
> +++ b/contrib/bloom/blscan.c
> @@ -13,14 +13,14 @@
> #include "postgres.h"
>
> #include "access/relscan.h"
> -#include "pgstat.h"
> +#include "bloom.h"
> #include "miscadmin.h"
> +#include "pgstat.h"
> #include "storage/bufmgr.h"
> #include "storage/lmgr.h"
> #include "utils/memutils.h"
> #include "utils/rel.h"
>
> -#include "bloom.h"
>
> /*
> * Begin scan of bloom index.
>
> The above changes lead to one extra line between the last header
> include and from where the actual code starts.
>
> 2.
> diff --git a/contrib/intarray/_int_bool.c b/contrib/intarray/_int_bool.c
> index 91e2a80..0d2f667 100644
> --- a/contrib/intarray/_int_bool.c
> +++ b/contrib/intarray/_int_bool.c
> @@ -3,11 +3,11 @@
> */
> #include "postgres.h"
>
> +#include "_int.h"
> +
> #include "miscadmin.h"
> #include "utils/builtins.h"
>
> -#include "_int.h"
> -
> PG_FUNCTION_INFO_V1(bqarr_in);
> PG_FUNCTION_INFO_V1(bqarr_out);
> PG_FUNCTION_INFO_V1(boolop);
> diff --git a/contrib/intarray/_int_gin.c b/contrib/intarray/_int_gin.c
> index 7aebfec..d6241d4 100644
> --- a/contrib/intarray/_int_gin.c
> +++ b/contrib/intarray/_int_gin.c
> @@ -3,11 +3,11 @@
> */
> #include "postgres.h"
>
> +#include "_int.h"
> +
> #include "access/gin.h"
> #include "access/stratnum.h"
>
> Why extra line after inclusion of _int.h?
>
> 3.
> diff --git a/contrib/intarray/_int_tool.c b/contrib/intarray/_int_tool.c
> index fde8d15..75ad04e 100644
> --- a/contrib/intarray/_int_tool.c
> +++ b/contrib/intarray/_int_tool.c
> @@ -5,10 +5,10 @@
>
> #include <limits.h>
>
> -#include "catalog/pg_type.h"
> -
> #include "_int.h"
>
> +#include "catalog/pg_type.h"
> +
>
> Why extra lines after both includes?
>
> 4.
> diff --git a/contrib/intarray/_intbig_gist.c b/contrib/intarray/_intbig_gist.c
> index 2a20abe..87ea86c 100644
> --- a/contrib/intarray/_intbig_gist.c
> +++ b/contrib/intarray/_intbig_gist.c
> @@ -3,12 +3,12 @@
> */
> #include "postgres.h"
>
> +#include "_int.h"
> +
> #include "access/gist.h"
> #include "access/stratnum.h"
> #include "port/pg_bitutils.h"
>
> -#include "_int.h"
> -
> #define GETENTRY(vec,pos) ((GISTTYPE *)
> DatumGetPointer((vec)->vector[(pos)].key))
> /*
> ** _intbig methods
> diff --git a/contrib/isn/isn.c b/contrib/isn/isn.c
> index 0c2cac7..36bb582 100644
> --- 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"
>
> Again extra spaces. I am not why you have extra spaces in a few cases.
>
> I haven't reviewed it completely, but generally, the changes seem to
> be fine. Please see if you can be consistent in extra space between
> includes. Kindly check the same throughout the patch.
>
Thanks for reviewing the patch.
I have made an updated patch with comments you have suggested.
I have split the patch into 3 patches so that the review can be simpler.
This patch also includes the changes suggested by Peter & Andres.
I had just seen seen Tom Lane's suggestions regarding submodule header
file, this patch contains fix based on Andres suggestions. Let me know
if that need to be changed, I can update it.
Should we make this changes only in master branch or should we make in
back branches also. If we decide for back branches, I will check if
this patch can apply in back branches and if this patch cannot be
directly applied I can make separate patch for the back branch and
send.
Please let me know your suggestions for any changes.
Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com