Re: VS 2015 support in src/tools/msvc - Mailing list pgsql-hackers

* Michael Paquier wrote:

> On Fri, Apr 8, 2016 at 10:05 PM, Andrew Dunstan <andrew@dunslane.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
'constchar *' 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