Re: VS 2015 support in src/tools/msvc

* Michael Paquier wrote:

> On Fri, Apr 8, 2016 at 10:05 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> ¥> On 04/08/2016 07:15 AM, Christian Ullrich wrote:

>>> Michael, none of your patches change this, so how does it ever build on
>>> your system?
>
> Luck. I am getting a warning but the code is able to somewhat compile:
> src/port/chklocale.c(230): warning C4013: 'GetLocaleInfoEx'
> undefined; assuming extern returning int
> [C:\Users\IEUser\git\postgres\libpgport.vcxproj]

Weird. This assumed declaration is __cdecl, the actual function is
__stdcall, and I think this should be guaranteed to crash.

> But that's clearly incorrect to get that. As you are saying, what we
> actually just need to do is bumping _WIN32_WINNT to 0x0600 when
> compiling with VS2015, meaning that the minimum build requirement for
> Postgres with VS2015 would be Windows Vista, and it would not be
> possible to compile it on XP or Windows server 2k3. As XP is already
> out of support, I think that this is an acceptable tradeoff, and it
> would still be possible to build Postgres on XP with older versions of
> Visual. Thoughts?

I think you confuse two things here, let's call them "build environment"
and "build platform". The build environment is whichever Windows SDK
(among other things) is installed; if it is a version for Vista or
later, that just means it has the declaration in the first place, and
has the import in kernel32.lib. The build platform is the OS the
compiler is run on; as long as you find a compiler that understands the
headers from your chosen SDK version, you can run it on Windows 95 if
both of you want.

Changing _WIN32_WINNT also affects, indirectly, on which platforms the
resulting binaries can run. Assume a macro that has an alternative
definition, conditioned on _WIN32_WINNT >= _WIN32_WINNT_VISTA, that uses
a function added in Vista. A binary built using this declaration, no
matter where and when, will not run on anything older.

> Anyway, attached are updated patches. This makes the warning go away
> from my side, so I guess that it should allow Andrew to compile the
> code.

Which brings us to the next problem:

src/port/chklocale.c(233): warning C4133: 'function': incompatible
types - from 'const char *' to 'LPCWSTR' [...\postgres.vcxproj]

I have no idea why I get this warning; I would have expected something
more like this:

localetest.cpp(26): error C2664: 'int
GetLocaleInfoEx(LPCWSTR,LCTYPE,LPWSTR,int)': cannot convert
argument 1 from 'const char *' to 'LPCWSTR'

Apparently the warning is triggered by type mismatches in pointer
arithmetic, although I can't see any here. Anyway, it concerns the first
argument in this call to GetLocaleInfoEx(), which here is a const char*.

According to the documentation and the prototype, however, it should be
an LPCWSTR, because this function is Unicode-only (has no A/W variants).
Unless LOCALE_IDEFAULTANSICODEPAGE also changes the interpretation of
this first argument to a single-byte string, which is not mentioned
anywhere in the documentation and makes no sense to begin with, I don't
think this has ever worked either.

I just tested it, and, of course, if I pass '(LPCWSTR)"de-DE"' (narrow
string cast to LPCWSTR), the call fails with ERROR_INVALID_PARAMETER.
With a wide string, I get the correct code page for the locale.

Also, while you're at it, could you improve the comments a bit? I have
not yet tried following the code to see which locale formats it uses
where ("German_Germany", "de-DE", etc.), but GetLocaleInfoEx() takes the
short form and there is a comment about the long form right below that
call once patched (in the old code that gets turned into an else branch).

--
Christian