Incorrect include file order in guc-file.l
Hi all,
While reviewing a different patch, I have noticed that guc-file.l
includes sys/stat.h in the middle of the PG internal headers. The
usual practice is to have first postgres[_fe].h, followed by the
system headers and finally the internal headers. That's a nit, but
all the other files do that.
{be,fe}-secure-openssl.c include some exceptions though, as documented
there.
Thoughts?
--
Michael
Attachments:
gucfile-include.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 2436396306..caa9c902b9 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -10,6 +10,7 @@
#include "postgres.h"
#include <ctype.h>
+#include <sys/stat.h>
#include <unistd.h>
#include "common/file_utils.h"
@@ -17,7 +18,6 @@
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "storage/fd.h"
-#include <sys/stat.h>
#include "utils/memutils.h"
}
Hi,
On Wed, Nov 02, 2022 at 02:29:50PM +0900, Michael Paquier wrote:
While reviewing a different patch, I have noticed that guc-file.l
includes sys/stat.h in the middle of the PG internal headers. The
usual practice is to have first postgres[_fe].h, followed by the
system headers and finally the internal headers. That's a nit, but
all the other files do that.{be,fe}-secure-openssl.c include some exceptions though, as documented
there.
Agreed, it's apparently an oversight in dac048f71eb. +1 for the patch.
On Wed, Nov 2, 2022 at 1:01 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
Hi,
On Wed, Nov 02, 2022 at 02:29:50PM +0900, Michael Paquier wrote:
While reviewing a different patch, I have noticed that guc-file.l
includes sys/stat.h in the middle of the PG internal headers. The
usual practice is to have first postgres[_fe].h, followed by the
system headers and finally the internal headers. That's a nit, but
all the other files do that.{be,fe}-secure-openssl.c include some exceptions though, as documented
there.Agreed, it's apparently an oversight in dac048f71eb. +1 for the patch.
Yeah. +1, thanks.
--
John Naylor
EDB: http://www.enterprisedb.com
On Wed, Nov 2, 2022 at 1:01 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
Hi,
On Wed, Nov 02, 2022 at 02:29:50PM +0900, Michael Paquier wrote:
While reviewing a different patch, I have noticed that guc-file.l
includes sys/stat.h in the middle of the PG internal headers. The
usual practice is to have first postgres[_fe].h, followed by the
system headers and finally the internal headers. That's a nit, but
all the other files do that.{be,fe}-secure-openssl.c include some exceptions though, as documented
there.Agreed, it's apparently an oversight in dac048f71eb. +1 for the patch.
I've pushed this, thanks!
--
John Naylor
EDB: http://www.enterprisedb.com
On Thu, Nov 03, 2022 at 12:40:19PM +0700, John Naylor wrote:
On Wed, Nov 2, 2022 at 1:01 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
Agreed, it's apparently an oversight in dac048f71eb. +1 for the patch.
I've pushed this, thanks!
Thanks for the commit. I've wanted to get it done yesterday but life
took over faster than that. Before committing the change, there is
something I have noticed though: this header does not seem to be
necessary at all and it looks that there is nothing in guc-file.l that
needs it. Why did you add it in dac048f to begin with?
--
Michael
On Thu, Nov 3, 2022 at 6:40 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Nov 03, 2022 at 12:40:19PM +0700, John Naylor wrote:
On Wed, Nov 2, 2022 at 1:01 PM Julien Rouhaud <rjuju123@gmail.com>
wrote:
Agreed, it's apparently an oversight in dac048f71eb. +1 for the patch.
I've pushed this, thanks!
Thanks for the commit. I've wanted to get it done yesterday but life
took over faster than that. Before committing the change, there is
something I have noticed though: this header does not seem to be
necessary at all and it looks that there is nothing in guc-file.l that
needs it. Why did you add it in dac048f to begin with?
Because it wouldn't compile otherwise, obviously. :-)
I must have been working on it before bfb9dfd93720
--
John Naylor
EDB: http://www.enterprisedb.com
On Thu, Nov 03, 2022 at 07:19:07PM +0700, John Naylor wrote:
Because it wouldn't compile otherwise, obviously. :-)
I must have been working on it before bfb9dfd93720
Hehe, my fault then ;p
The CI is able to complete without it. Would you mind if it is
removed? If you don't want us to poke more at the bear, that's a nit
so leaving things as they are is also fine by me.
--
Michael
On Fri, Nov 4, 2022 at 5:42 AM Michael Paquier <michael@paquier.xyz> wrote:
The CI is able to complete without it. Would you mind if it is
removed? If you don't want us to poke more at the bear, that's a nit
so leaving things as they are is also fine by me.
I've removed it.
--
John Naylor
EDB: http://www.enterprisedb.com