Further simplification of c.h's #include section
Yesterday's commit 91aec93e6 got rid of quite a bit of unprincipled cruft
that had accumulated in c.h's #include section. It occurs to me that we
could clean it up some more, eliminating almost all the Windows-specific
hacking there, by doing this:
1. Move what's currently in src/include/port/win32.h (a/k/a
pg_config_os.h) to a new file, say src/include/port/win32_2.h.
2. Move these bits of c.h into win32.h:
#if defined(_WIN32) && !defined(WIN32)
#define WIN32
#endif
#if _MSC_VER >= 1400 || defined(HAVE_CRTDEFS_H)
#define errcode __msvc_errcode
#include <crtdefs.h>
#undef errcode
#endif
3. Then the #include for pg_config_os.h just becomes unconditional and
uncluttered.
4. Below the system #includes, where we have
#if defined(WIN32) || defined(__CYGWIN__)
/* We have to redefine some system functions after they are included above. */
#include "pg_config_os.h"
#endif
I'd propose just changing that to include "port/win32_2.h".
Aside from being cleaner, this would provide a clear framework for other
platforms to inject code both before and after the system headers.
To make that happen, we'd have to set up pg_config_os_2.h symlinks and
replace the above-quoted bit with #include "pg_config_os_2.h". I don't
feel a need to make that happen right now, but it'd be straightforward
to do it if we need to.
Once that's done, there might be reason to rethink the division of
code between win32.h and win32_2.h, but not being a Windows hacker
I'm not qualified to do that.
Thoughts, objections?
regards, tom lane
On Wed, Nov 15, 2017 at 10:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
1. Move what's currently in src/include/port/win32.h (a/k/a
pg_config_os.h) to a new file, say src/include/port/win32_2.h.
I have no objection to trying to clean things up in that area, but I
request a less-lame filename than win32_2.h
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Nov 15, 2017 at 10:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
1. Move what's currently in src/include/port/win32.h (a/k/a
pg_config_os.h) to a new file, say src/include/port/win32_2.h.
I have no objection to trying to clean things up in that area, but I
request a less-lame filename than win32_2.h
Sure, if you have a suggestion.
regards, tom lane
I wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Nov 15, 2017 at 10:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
1. Move what's currently in src/include/port/win32.h (a/k/a
pg_config_os.h) to a new file, say src/include/port/win32_2.h.
I have no objection to trying to clean things up in that area, but I
request a less-lame filename than win32_2.h
Sure, if you have a suggestion.
How do you feel about "win32_more.h"?
regards, tom lane
On Wed, Nov 15, 2017 at 4:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I have no objection to trying to clean things up in that area, but I
request a less-lame filename than win32_2.hSure, if you have a suggestion.
How do you feel about "win32_more.h"?
Seems morally equivalent to what you had before. I think what I would
be looking for is a filename that somehow conveys what the difference
is between what should go in the existing file and what should go in
the new file. If we don't know, maybe we should find out before we
change things.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Nov 15, 2017 at 4:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
How do you feel about "win32_more.h"?
Seems morally equivalent to what you had before. I think what I would
be looking for is a filename that somehow conveys what the difference
is between what should go in the existing file and what should go in
the new file. If we don't know, maybe we should find out before we
change things.
Well, the point is whether it gets included before or after the key
system header files. "win32_post_headers.h", perhaps?
As for the question of what actually needs to be in it, you're
asking the wrong person. It looks like some of it could be moved
to before the system headers, but I am in no position to find out
exactly what, except by trial-and-error with the buildfarm. And
TBH I don't care particularly, as long as it's in a windows-specific
file and not a common file.
regards, tom lane
I wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Nov 15, 2017 at 4:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
How do you feel about "win32_more.h"?
Seems morally equivalent to what you had before. I think what I would
be looking for is a filename that somehow conveys what the difference
is between what should go in the existing file and what should go in
the new file. If we don't know, maybe we should find out before we
change things.
Well, the point is whether it gets included before or after the key
system header files. "win32_post_headers.h", perhaps?
Actually, on closer inspection, it seems like there's no need for
a windows-specific #include right there at all. We could move
almost everything that's currently in win32.h to be done in port.h,
at the bottom of c.h rather than at the top. The only exception
is stuff that would affect #if decisions taken in c.h itself, which
it looks like is only PGDLLIMPORT/PGDLLEXPORT, and those two could
perfectly well be declared before importing system headers.
Now, dropping everything in win32.h into port.h is surely no improvement,
but it seems like we could move all that stuff to a new file
"win32_port.h" and have the bottom of c.h look like
/* Windows-specific compatibility functions */
#if defined(WIN32) || defined(__CYGWIN__)
#include "win32_port.h"
#endif
/* Generic compatibility functions */
#include "port.h"
or else make the new file a sub-include of port.h.
There's also some fair-size stanzas in port.h that could arguably
be moved into win32_port.h if we did it like this. I think the
parts that are #if WIN32 something #else something-else #endif
are fine as-is, but the parts that are just WIN32 without any
corresponding non-Windows code could be moved.
Thoughts?
regards, tom lane
On Wed, Nov 15, 2017 at 5:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thoughts?
Sure, having a win32_port.h as a sub-include of port.h seems fine.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Nov 15, 2017 at 5:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thoughts?
Sure, having a win32_port.h as a sub-include of port.h seems fine.
Here's a draft patch for this. I'm not too certain about the interactions
with Cygwin; some of the stuff I moved out of port.h might have to go back
there so that a Cygwin build will see it. There might also be some
declaration ordering dependencies that I failed to spot.
(Speaking of which, I'm wondering why the existing code monkeys around
with _WIN32_WINNT after it's already included a bunch of system headers.
Shouldn't that be set earlier --- in other words, shouldn't that code
move back to win32.h from win32_port.h? But I've not touched that here.
I did remove an at-best-redundant definition from pg_ctl.c though.)
Anybody want to test this manually, or shall we just throw it into the
buildfarm and see what blows up?
regards, tom lane
Attachments:
saner-win32-header-setup-1.patchtext/x-diff; charset=us-ascii; name=saner-win32-header-setup-1.patchDownload+560-513
On 16 November 2017 at 15:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Anybody want to test this manually, or shall we just throw it into the
buildfarm and see what blows up?
I don't have any cygwin around, but it compiles fine with the Visual
Studios 2012 toolchain.
I do get some warnings:
(ClCompile target) ->
src/test/modules/test_session_hooks/test_session_hooks.c(112): warning
C4113: 'void (__cdecl *)()' differs in parameter lists from
'session_start_hook_type' [test_session_hooks.vcxproj]
src/test/modules/test_session_hooks/test_session_hooks.c(113): warning
C4113: 'void (__cdecl *)()' differs in parameter lists from
'session_end_hook_type' [test_session_hooks.vcxproj].
2 Warning(s)
But I imagine that was caused by 745948422c (also get the warnings without
your patch)
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Nov 16, 2017 at 2:23 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
I do get some warnings:
(ClCompile target) ->
src/test/modules/test_session_hooks/test_session_hooks.c(112): warning
C4113: 'void (__cdecl *)()' differs in parameter lists from
'session_start_hook_type' [test_session_hooks.vcxproj]
src/test/modules/test_session_hooks/test_session_hooks.c(113): warning
C4113: 'void (__cdecl *)()' differs in parameter lists from
'session_end_hook_type' [test_session_hooks.vcxproj].2 Warning(s)
But I imagine that was caused by 745948422c (also get the warnings without
your patch)
Do the warnings go away with the patch attached?
--
Michael
Attachments:
session_hook_void.patchapplication/octet-stream; name=session_hook_void.patchDownload+2-2
On 16 November 2017 at 19:14, Michael Paquier <michael.paquier@gmail.com> wrote:
On Thu, Nov 16, 2017 at 2:23 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:I do get some warnings:
(ClCompile target) ->
src/test/modules/test_session_hooks/test_session_hooks.c(112): warning
C4113: 'void (__cdecl *)()' differs in parameter lists from
'session_start_hook_type' [test_session_hooks.vcxproj]
src/test/modules/test_session_hooks/test_session_hooks.c(113): warning
C4113: 'void (__cdecl *)()' differs in parameter lists from
'session_end_hook_type' [test_session_hooks.vcxproj].2 Warning(s)
But I imagine that was caused by 745948422c (also get the warnings without
your patch)Do the warnings go away with the patch attached?
Yes
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes:
On 16 November 2017 at 15:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Anybody want to test this manually, or shall we just throw it into the
buildfarm and see what blows up?
I don't have any cygwin around, but it compiles fine with the Visual
Studios 2012 toolchain.
Thanks for testing! I pushed it after a little bit more cosmetic
rearrangement; in particular, I ended up deciding that the _WIN32_WINNT
business really ought to be before the system headers, even if we've
somehow got away without that all these years. The buildfarm will
soon tell us if I was right or not.
regards, tom lane