Ordering of header file inclusion

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Ordering of header file inclusion

vignesh C
Hi,

I noticed that some of the header files inclusion is not ordered as
per the usual standard that is followed.
The attached patch contains the fix for the order in which the header
files are included.
Let me know your thoughts on the same.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

0001-Ordering-of-header-file-inclusion.patch (34K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

akapila
On Wed, Oct 2, 2019 at 2:57 PM vignesh C <[hidden email]> wrote:
>
> Hi,
>
> I noticed that some of the header files inclusion is not ordered as
> per the usual standard that is followed.
> The attached patch contains the fix for the order in which the header
> files are included.
> Let me know your thoughts on the same.
>

+1.   I think this will make an order of header inclusions consistent
throughout code.   One thing which will be slightly tricky is we might
not be able to back-patch this as some of this belongs to a recent
version(s) and others to older versions as well.   OTOH, I have not
investigated how much of this is relevant to back branches.  I think
most of these will apply to 12, but I am not sure if it is worth the
effort to segregate the changes which apply to back branches.  What do
you think?

 Few minor comments after a quick read:
 #include "lib/ilist.h"
-
+#include "miscadmin.h"

I think we shouldn't remove the extra line as part of the above change.

--- a/src/bin/psql/variables.c
+++ b/src/bin/psql/variables.c
@@ -8,10 +8,8 @@
 #include "postgres_fe.h"

 #include "common.h"
-#include "variables.h"
-
 #include "common/logging.h"
-
+#include "variables.h"

Same as above.


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


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

Tom Lane-2
Amit Kapila <[hidden email]> writes:
> On Wed, Oct 2, 2019 at 2:57 PM vignesh C <[hidden email]> wrote:
>> I noticed that some of the header files inclusion is not ordered as
>> per the usual standard that is followed.
>> The attached patch contains the fix for the order in which the header
>> files are included.
>> Let me know your thoughts on the same.

> +1.

FWIW, I'm not on board with reordering system-header inclusions.
Some platforms have (had?) ordering dependencies for those, and where
that's true, it's seldom alphabetical.  It's only our own headers
where we can safely expect that any arbitrary order will work.

> I think we shouldn't remove the extra line as part of the above change.

I would take out the blank lines between our own #includes.  Those are
totally arbitrary and unnecessary.  The whole point of style rules like
this one is to reduce the differences between the way one person might
write something and the way that someone else might.  Deciding to throw
in a blank line is surely in the realm of unnecessary differences.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

akapila
On Tue, Oct 8, 2019 at 8:19 PM Tom Lane <[hidden email]> wrote:

>
> Amit Kapila <[hidden email]> writes:
> > On Wed, Oct 2, 2019 at 2:57 PM vignesh C <[hidden email]> wrote:
> >> I noticed that some of the header files inclusion is not ordered as
> >> per the usual standard that is followed.
> >> The attached patch contains the fix for the order in which the header
> >> files are included.
> >> Let me know your thoughts on the same.
>
> > +1.
>
> FWIW, I'm not on board with reordering system-header inclusions.
> Some platforms have (had?) ordering dependencies for those, and where
> that's true, it's seldom alphabetical.  It's only our own headers
> where we can safely expect that any arbitrary order will work.
>

Okay, that makes sense.  However, I noticed that ordering for
system-header inclusions is somewhat random.  For ex. nodeSubPlan.c,
datetime.c, etc. include limits.h first and then math.h whereas
knapsack.c, float.c includes them in reverse order.  There could be
more such inconsistencies and the probable reason is that we don't
have any specific rule, so different people decide to do it
differently.

> > I think we shouldn't remove the extra line as part of the above change.
>
> I would take out the blank lines between our own #includes.
>

Okay, that would be better, but doing it half-heartedly as done in
patch might make it worse.  So, it is better to remove blank lines
between our own #includes in all cases.

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


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

vignesh C
On Wed, Oct 9, 2019 at 11:37 AM Amit Kapila <[hidden email]> wrote:

>
> On Tue, Oct 8, 2019 at 8:19 PM Tom Lane <[hidden email]> wrote:
> >
> > Amit Kapila <[hidden email]> writes:
> > > On Wed, Oct 2, 2019 at 2:57 PM vignesh C <[hidden email]> wrote:
> > >> I noticed that some of the header files inclusion is not ordered as
> > >> per the usual standard that is followed.
> > >> The attached patch contains the fix for the order in which the header
> > >> files are included.
> > >> Let me know your thoughts on the same.
> >
> > > +1.
> >
> > FWIW, I'm not on board with reordering system-header inclusions.
> > Some platforms have (had?) ordering dependencies for those, and where
> > that's true, it's seldom alphabetical.  It's only our own headers
> > where we can safely expect that any arbitrary order will work.
> >
>
> Okay, that makes sense.  However, I noticed that ordering for
> system-header inclusions is somewhat random.  For ex. nodeSubPlan.c,
> datetime.c, etc. include limits.h first and then math.h whereas
> knapsack.c, float.c includes them in reverse order.  There could be
> more such inconsistencies and the probable reason is that we don't
> have any specific rule, so different people decide to do it
> differently.
>
> > > I think we shouldn't remove the extra line as part of the above change.
> >
> > I would take out the blank lines between our own #includes.
> >
>
> Okay, that would be better, but doing it half-heartedly as done in
> patch might make it worse.  So, it is better to remove blank lines
> between our own #includes in all cases.
>
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.
I have removed the duplicate includes of certain header files in same
source file.
In some place postgres header files was getting included as
<postgres_header.h>, I have changed it to "postgres_header.h".
Let me know if any change is required.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

Ordering-of-header-files-15thOctober.patch (288K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

akapila
On Tue, Oct 15, 2019 at 10:57 PM vignesh C <[hidden email]> wrote:

>
> On Wed, Oct 9, 2019 at 11:37 AM Amit Kapila <[hidden email]> wrote:
> >
> > On Tue, Oct 8, 2019 at 8:19 PM Tom Lane <[hidden email]> wrote:
> > >
> > > Amit Kapila <[hidden email]> writes:
> > > > On Wed, Oct 2, 2019 at 2:57 PM vignesh C <[hidden email]> wrote:
> > > >> I noticed that some of the header files inclusion is not ordered as
> > > >> per the usual standard that is followed.
> > > >> The attached patch contains the fix for the order in which the header
> > > >> files are included.
> > > >> Let me know your thoughts on the same.
> > >
> > > > +1.
> > >
> > > FWIW, I'm not on board with reordering system-header inclusions.
> > > Some platforms have (had?) ordering dependencies for those, and where
> > > that's true, it's seldom alphabetical.  It's only our own headers
> > > where we can safely expect that any arbitrary order will work.
> > >
> >
> > Okay, that makes sense.  However, I noticed that ordering for
> > system-header inclusions is somewhat random.  For ex. nodeSubPlan.c,
> > datetime.c, etc. include limits.h first and then math.h whereas
> > knapsack.c, float.c includes them in reverse order.  There could be
> > more such inconsistencies and the probable reason is that we don't
> > have any specific rule, so different people decide to do it
> > differently.
> >
> > > > I think we shouldn't remove the extra line as part of the above change.
> > >
> > > I would take out the blank lines between our own #includes.
> > >
> >
> > Okay, that would be better, but doing it half-heartedly as done in
> > patch might make it worse.  So, it is better to remove blank lines
> > between our own #includes in all cases.
> >
> Attached patch contains the fix based on the comments suggested.
>

Thanks for working on this.  I will look into this in the coming few
days or during next CF.  Can you please register it for the next CF
(https://commitfest.postgresql.org/25/)?

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


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

vignesh C
On Wed, Oct 16, 2019 at 8:10 AM Amit Kapila <[hidden email]> wrote:
>
> Thanks for working on this.  I will look into this in the coming few
> days or during next CF.  Can you please register it for the next CF
> (https://commitfest.postgresql.org/25/)?
>
Thanks, I have added it to the commitfest.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

akapila
In reply to this post by vignesh C
On Tue, Oct 15, 2019 at 10:57 PM vignesh C <[hidden email]> 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.

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


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

Peter Eisentraut-6
In reply to this post by vignesh C
diff --git a/contrib/bloom/blcost.c b/contrib/bloom/blcost.c
index f9fe57f..6224735 100644
--- a/contrib/bloom/blcost.c
+++ b/contrib/bloom/blcost.c
@@ -12,10 +12,10 @@
  */
 #include "postgres.h"

+#include "bloom.h"
 #include "fmgr.h"
 #include "utils/selfuncs.h"

-#include "bloom.h"

 /*
  * Estimate cost of bloom index scan.

This class of change I don't like.

The existing arrangement keeps "other" header files separate from the
header file of the module itself.  It seems useful to keep that separate.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

Andres Freund
Hi,

On 2019-10-19 21:50:03 +0200, Peter Eisentraut wrote:

> diff --git a/contrib/bloom/blcost.c b/contrib/bloom/blcost.c
> index f9fe57f..6224735 100644
> --- a/contrib/bloom/blcost.c
> +++ b/contrib/bloom/blcost.c
> @@ -12,10 +12,10 @@
>   */
>  #include "postgres.h"
>
> +#include "bloom.h"
>  #include "fmgr.h"
>  #include "utils/selfuncs.h"
>
> -#include "bloom.h"
>
>  /*
>   * Estimate cost of bloom index scan.
>
> This class of change I don't like.
>
> The existing arrangement keeps "other" header files separate from the
> header file of the module itself.  It seems useful to keep that separate.

If we were to do so, we ought to put bloom.h first and clearly seperated
out, not last, as the former makes the bug of the the header not being
standalone more obvious.

I'm -1 on having a policy of putting the headers separate though, I feel
that's too much work, and there's too many cases where it's not that
clear which header that should be.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2019-10-19 21:50:03 +0200, Peter Eisentraut wrote:
>> This class of change I don't like.
>> The existing arrangement keeps "other" header files separate from the
>> header file of the module itself.  It seems useful to keep that separate.

> If we were to do so, we ought to put bloom.h first and clearly seperated
> out, not last, as the former makes the bug of the the header not being
> standalone more obvious.

We have headerscheck and cpluspluscheck to catch that problem, so I don't
think that it needs to be a reason not to rationalize header inclusion
order.

I don't have a very strong opinion on whether modules outside the core
backend should separate their own headers from core-system headers.
I think there's some argument for that, but it's not something we've
done consistently.  And, as you say, there's no convention as to
where we'd include local headers if we do separate them.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

vignesh C
In reply to this post by akapila
On Thu, Oct 17, 2019 at 4:44 PM Amit Kapila <[hidden email]> wrote:

>
> On Tue, Oct 15, 2019 at 10:57 PM vignesh C <[hidden email]> 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

0001-Ordering-of-header-files-in-contrib-dir.patch (57K) Download Attachment
0003-Ordering-of-header-files-remaining-dir.patch (87K) Download Attachment
0002-Ordering-of-header-files-in-backend-dir.patch (160K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

vignesh C
In reply to this post by Peter Eisentraut-6
On Sun, Oct 20, 2019 at 1:20 AM Peter Eisentraut
<[hidden email]> wrote:

>
> diff --git a/contrib/bloom/blcost.c b/contrib/bloom/blcost.c
> index f9fe57f..6224735 100644
> --- a/contrib/bloom/blcost.c
> +++ b/contrib/bloom/blcost.c
> @@ -12,10 +12,10 @@
>   */
>  #include "postgres.h"
>
> +#include "bloom.h"
>  #include "fmgr.h"
>  #include "utils/selfuncs.h"
>
> -#include "bloom.h"
>
>  /*
>   * Estimate cost of bloom index scan.
>
> This class of change I don't like.
>
> The existing arrangement keeps "other" header files separate from the
> header file of the module itself.  It seems useful to keep that separate.
>
Thanks Peter for your thoughts, I have modified the changes based on
your suggestions.
I have included the module header file in the beginning.
The changes are attached in the previous mail.

Please let me know your suggestions for any changes.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

vignesh C
In reply to this post by Andres Freund
On Sun, Oct 20, 2019 at 2:44 AM Andres Freund <[hidden email]> wrote:

>
> Hi,
>
> On 2019-10-19 21:50:03 +0200, Peter Eisentraut wrote:
> > diff --git a/contrib/bloom/blcost.c b/contrib/bloom/blcost.c
> > index f9fe57f..6224735 100644
> > --- a/contrib/bloom/blcost.c
> > +++ b/contrib/bloom/blcost.c
> > @@ -12,10 +12,10 @@
> >   */
> >  #include "postgres.h"
> >
> > +#include "bloom.h"
> >  #include "fmgr.h"
> >  #include "utils/selfuncs.h"
> >
> > -#include "bloom.h"
> >
> >  /*
> >   * Estimate cost of bloom index scan.
> >
> > This class of change I don't like.
> >
> > The existing arrangement keeps "other" header files separate from the
> > header file of the module itself.  It seems useful to keep that separate.
>
> If we were to do so, we ought to put bloom.h first and clearly seperated
> out, not last, as the former makes the bug of the the header not being
> standalone more obvious.
>
> I'm -1 on having a policy of putting the headers separate though, I feel
> that's too much work, and there's too many cases where it's not that
> clear which header that should be.
>
Thanks Andres for reviewing the changes, I have modified the changes
based on your suggestions.
I have included the module header file in the beginning.
The changes are attached in the previous mail.

Please let me know your suggestions for any changes.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

akapila
In reply to this post by vignesh C
On Sun, Oct 20, 2019 at 10:58 PM vignesh C <[hidden email]> wrote:

>
> On Thu, Oct 17, 2019 at 4:44 PM Amit Kapila <[hidden email]> wrote:
> >
> >
> > 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.
>

AFAICS, none of Andres or Tom seems to be in favor of separating
module headers.  I am also not sure if we should try to make sure of
that in every case.

> Should we make this changes only in master branch or should we make in
> back branches also.
>

I am in favor of doing this only for HEAD, but I am fine if others
want to see for back branches as well and you can prepare the patches
for the same.

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


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

Tom Lane-2
Amit Kapila <[hidden email]> writes:
> On Sun, Oct 20, 2019 at 10:58 PM vignesh C <[hidden email]> wrote:
>> Should we make this changes only in master branch or should we make in
>> back branches also.

> I am in favor of doing this only for HEAD, but I am fine if others
> want to see for back branches as well and you can prepare the patches
> for the same.

There is no good reason to back-patch this.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

vignesh C
In reply to this post by akapila
On Mon, Oct 21, 2019 at 8:47 AM Amit Kapila <[hidden email]> wrote:

>
> On Sun, Oct 20, 2019 at 10:58 PM vignesh C <[hidden email]> wrote:
> >
> > On Thu, Oct 17, 2019 at 4:44 PM Amit Kapila <[hidden email]> wrote:
> > >
> > >
> > > 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.
> >
>
> AFAICS, none of Andres or Tom seems to be in favor of separating
> module headers.  I am also not sure if we should try to make sure of
> that in every case.
>
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.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

0001-Ordering-of-header-files-in-contrib-dir-oct21.patch (57K) Download Attachment
0003-Ordering-of-header-files-remaining-dir-oct21.patch (84K) Download Attachment
0002-Ordering-of-header-files-in-backend-dir-oct21.patch (160K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

akapila
On Mon, Oct 21, 2019 at 11:04 PM vignesh C <[hidden email]> wrote:
>
> On Mon, Oct 21, 2019 at 8:47 AM Amit Kapila <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

akapila
On Tue, Oct 22, 2019 at 12:56 PM Amit Kapila <[hidden email]> wrote:

>
> On Mon, Oct 21, 2019 at 11:04 PM vignesh C <[hidden email]> wrote:
> >
> > On Mon, Oct 21, 2019 at 8:47 AM Amit Kapila <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Ordering of header file inclusion

vignesh C
In reply to this post by akapila
On Tue, Oct 22, 2019 at 12:56 PM Amit Kapila <[hidden email]> 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

0001-Ordering-of-header-files-in-contrib-dir-oct22.patch (58K) Download Attachment
12