Ordering of header file inclusion

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
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