Re: Ordering of header file inclusion
От | vignesh C |
---|---|
Тема | Re: Ordering of header file inclusion |
Дата | |
Msg-id | CALDaNm3qVvqOP_7HbGAVx_R2JYh97EbePy-hQ1Of1JefZExQmQ@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Ordering of header file inclusion (Amit Kapila <amit.kapila16@gmail.com>) |
Ответы |
Re: Ordering of header file inclusion
(Amit Kapila <amit.kapila16@gmail.com>)
|
Список | pgsql-hackers |
On Thu, Oct 17, 2019 at 4:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Oct 15, 2019 at 10:57 PM vignesh C <vignesh21@gmail.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
Вложения
В списке pgsql-hackers по дате отправления: