Re: PG compilation error with Visual Studio 2015/2017/2019

Em ter., 21 de abr. de 2020 às 09:02, Juan José Santamaría Flecha <
juanjo(dot)santamaria(at)gmail(dot)com> escreveu:

>
> On Tue, Apr 21, 2020 at 12:41 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>> On Tue, Apr 21, 2020 at 12:51 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>> >
>> > I have tried a simple test with the latest patch and it failed for me.
>> >
>> > Set LC_MESSAGES="English_United Kingdom";
>> > -- returns en-GB, then code changes it to en_NZ when _create_locale()
>> > is used whereas with the patch it returns "" (empty string).
>> >
>> > There seem to be two problems here (a) The input to enum_locales_fn
>> > doesn't seem to get the input name as "English_United Kingdom" due to
>> > which it can't find a match even if the same exists. (b) After
>> > executing EnumSystemLocalesEx, there is no way the patch can detect if
>> > it is successful in finding the passed name due to which it appends
>> > empty string in such cases.
>>
>> Few more comments:
>> 1. I have tried the first one in the list provided by you and that
>> also didn't work. Basically, I got empty string when I tried Set
>> LC_MESSAGES='Afar';
>>
>
> I cannot reproduce any of these errors on my end. When using
> _create_locale(), returning "en_NZ" is also a wrong result.
>
>
>> 2. Getting below warning
>> pg_locale.c(1072): warning C4133: 'function': incompatible types -
>> from 'const char *' to 'const wchar_t *'
>>
>
> Yes, that is a regression.
>
>
>> 3.
>> + if (GetLocaleInfoEx(pStr, LOCALE_SENGLISHCOUNTRYNAME,
>> + test_locale + len, LOCALE_NAME_MAX_LENGTH - len) > 0)
>>
>> All > or <= 0 checks should be changed to "!" types which mean to
>> check whether the call toGetLocaleInfoEx is success or not.
>>
>
> MSVC does not recommend "!" in all cases, but GetLocaleInfoEx() looks
> fine, so agreed.
>
> 4. In the patch, first, we try to get with LCType as LOCALE_SNAME and
>> then with LOCALE_SENGLISHLANGUAGENAME and LOCALE_SENGLISHCOUNTRYNAME.
>> I think we should add comments indicating why we try to get the locale
>> information with three LCTypes and why the specific order of trying
>> those types is required.
>>
>
> Agreed.
>
>
>> 5. In one of the previous emails, you asked whether we have a list of
>> supported locales. I don't find any such list. I think it depends on
>> Windows locales for which you can get the information from
>>
>> https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-lcid/a9eac961-e77d-41a6-90a5-ce1a8b0cdb9c
>
>
> Yes, that is the information we get from EnumSystemLocalesEx(), without
> the additional entries _create_locale() has.
>
> Please find attached a new version addressing the above mentioned, and so
> adding a debug message for trying to get more information on the failed
> cases.
>
More few comments.

1. Comments about order:
/*
* Callback function for EnumSystemLocalesEx.
* Stop enumerating if a match is found for a locale with the format
* <Language>_<Country>.
* The order for search locale is essential:
* Find LCType first as LOCALE_SNAME, if not found try
LOCALE_SENGLISHLANGUAGENAME and
* finally LOCALE_SENGLISHCOUNTRYNAME, before return.
*/

Typo "enumarating".

2. Maybe the fail has here:

if (hyphen == NULL || underscore == NULL)

Change || to &&, the logical is wrong?

3. Why iso_lc_messages[0] = '\0'?

If we go call strchr, soon after, it's a waste.

regards,
Ranier Vilela