Re: VS 2015 support in src/tools/msvc

On Mon, Apr 11, 2016 at 6:03 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> On 10/04/16 20:47, Christian Ullrich wrote:
>>> don't think we need to be too precious about saving a byte or two
>>> here. Can one or other of you please prepare a replacement patch based
>>> in this discussion?

So, I am finally back on the battlefield.

>> Sorry, I don't think I'm up to that (at least not for another week or
>> so). I have to read up on the issue first; right now I'm not sure what
>> exactly the problem is.
>>
>> What I can say is that the existing patch needs more work, because
>> GetLocaleInfoEx() expects a locale name ("de-DE") as its first argument,
>> but the patched win32_langinfo() is giving it a locale identifier
>> ("German_Germany.1252"). At least it does for me.
>
>
> That really depends on what you set in config/commandline. The current code
> supports both (that's why there is the else fallback to old code which
> handles the "German_Germany.1252" format).

Yes, that's the whole point of falling back to the old code path
should the call to GetLocaleInfoEx() fail.

>> As for missing code page information in the _locale_t type, ISTM it
>> isn't hidden any better in UCRT than it was before:
>>
>> int main()
>> {
>> /* Set locale with nonstandard code page */
>> _locale_t loc = _create_locale(LC_ALL, "German_Germany.850");
>>
>> __crt_locale_data_public* pub =
>> (__crt_locale_data_public*)(loc->locinfo);
>> printf("CP: %d\n", pub->_locale_lc_codepage); // "CP: 850"
>> return 0;
>> }
>>
>
> This is basically same as the approach of fixing _locale_t that was also
> considered upthread but nobody was too happy with it.

I am one of them to be honest... Now if I look at that with one step
backward at this problem this requires just a ugly hack of a couple of
lines, and this does not require to bump __WIN32_WINNT... Neither does
it need an extra code hunk to handle the short locale name parsing
with GetLocaleInfoEx. We could even just handle _MSC_VER as an
exception, so as it is easy to check with future versions of VS if we
still have lc_codepage going missing:
+#if (_MSC_VER == 1900)
+ __crt_locale_data_public *pub =
+ (__crt_locale_data_public *) loct->locinfo;
+ lc_codepage = pub->_locale_lc_codepage;
+#else
+ lc_codepage = loct->locinfo->lc_codepage;
+#endif

Another thing that I am wondering is that this would allow us to parse
even locale names that are not short, type ja-JP and not with the
COUNTRY_LANG.CODEPAGE format, though based on what I read on the docs
those do not exist.. But the world of Windows is full of surprises.

> I guess the worry is
> that given that the header file is obviously unfinished in the area where
> this is defined and also the fact that this looks like something that's not
> meant to be used outside of that header makes people worry that it might
> change between point releases of the SDK.

Yep.

Now, I have produced two patches:
- 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
__crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
hack, though I am coming to think that this may be the approach that
would us the less harm, and that's closer to what is done for VS 2012
and 2013.
- 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
GetLocaleInfoEx, this requires us to lower a bit the support grid for
Windows, basically that's throwing support for XP if compilation is
done with VS 2015.
Based on my tests, both are working with short and long local names,
testing via initdb --locale.
--
Michael