Re: PG compilation error with Visual Studio 2015/2017/2019 - Mailing list pgsql-hackers

On Tue, Apr 21, 2020 at 5:32 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
> On Tue, Apr 21, 2020 at 12:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Tue, Apr 21, 2020 at 12:51 PM Amit Kapila <amit.kapila16@gmail.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.
>

The first problem related to the English_United Kingdom was due to the
usage of wcslen instead of pg_mbstrlen to compute the length of
winlocname.  So, this is fixed with your latest patch.  I have
debugged the case for 'Afar' and found that _create_locale also didn't
return anything for that in my machine, so probably that locale
information is not there in my environment.

>  When using _create_locale(),  returning "en_NZ" is also a wrong result.
>

Hmm, that was a typo, it should be en_GB instead.

>
>> 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.
>

But, I don't see much in the comments?

Few more comments:
1.
  if (rc == -1 || rc == sizeof(iso_lc_messages))
- return NULL;
+
iso_lc_messages[0] = '\0';

I don't think this change is required.  The caller expects NULL in
case the API is not successful so that it can point result directly to
the locale passed.  I have changed this back to the original code in
the attached patch.

2.
I see some differences in the output of GetLocaleInfoEx and
_create_locale for some locales as mentioned in one of the documents
shared by you. Ex.

Bemba_Zambia bem-ZM bem
Bena_Tanzania bez-TZ bez
Bulgarian_Bulgaria bg-BG bg

Now, these might be okay but I think unless we test such things by
seeing the error message changes due to these locales we can't be
sure.

3. In the attached patch, I have handled one of the problem reported
earlier aka "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."

4. I think for the matter of this API, it is better to use _MSC_VER
related checks instead of _WIN32_WINNT so as to be consistent with
similar usage in chklocale.c (see win32_langinfo).  We can later
change the checks at all places to _WIN32_WINNT if required.  I have
changed this as well in the attached patch.

5. I am slightly nervous about the usage of wchar functions like
_wcsicmp, wcslen, etc. as those are not used anywhere in the code.
OTOH, I don't see any problem with that.  There is pg_wchar datatype
in the code and some corresponding functions to manipulate it.  Have
you considered using it?

6. I have additionally done some cosmetic changes in the attached patch.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com