Thomas Munro <thomas.munro@gmail.com> writes:
> Here are some more, a couple of which I posted before but I've now
> gone a bit further with them in terms of removing configure checks
> etc:
After looking through these briefly, I'm pretty concerned about
whether this won't break our Cygwin build in significant ways.
For example, lorikeet reports "HAVE_SETSID 1", a condition that
you want to replace with !WIN32. The question here is whether
or not WIN32 is defined in a Cygwin build. I see some places
in our code that believe it is not, but others that believe that
it is --- and the former ones are mostly like
#if defined(__CYGWIN__) || defined(WIN32)
which means they wouldn't actually fail if they are wrong about that.
More generally, I'm not exactly convinced that changes like
this are a readability improvement:
-#ifdef HAVE_SETSID
+#ifndef WIN32
I'd rather not have the code cluttered with a sea of
indistinguishable "#ifndef WIN32" tests when some of them could be
more specific and more mnemonic. So I think we'd be better off
leaving that as-is. I don't mind nuking the configure-time test
and hard-wiring "#define HAVE_SETSID 1" somewhere, but changing
the code's #if tests doesn't seem to bring any advantage.
Specific to 0001, I don't especially like what you did to
src/port/dlopen.c. The original intent (and reality until
not so long ago) was that that would be a container for
various dlopen replacements. Well, okay, maybe there will
never be any more besides Windows, but I think then we should
either rename the file to (say) win32dlopen.c or move it to
src/backend/port/win32. Likewise for link.c in 0007 and
pread.c et al in 0011. (But 0010 is fine, because the
replacement code is already handled that way.)
OTOH, 0012 seems to immediately change pread.c et al back
to NOT being Windows-only, though it's hard to tell for
sure because the configure support seems all wrong.
I'm quite confused by those two patches ... are they really
correct?
regards, tom lane