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

On Wed, Apr 22, 2020 at 9:27 PM Juan José Santamaría Flecha
<juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>
>
> On Wed, Apr 22, 2020 at 1:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>>
>> >> 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?
>
>
> I take notice.
>

Okay, I hope we will see better comments in the next version.

>>
>> 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.
>
>
> I did not want to return anything without logging its value.
>

Hmm, if you really want to log the value then do it in the caller. I
don't think making special arrangements just for logging this value is
a good idea.

>>
>> 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.
>
>
> There are some cases where the language tag does not match, although I do not think is wrong:
>
> Asu asa Asu
> Edo bin Edo
> Ewe ee Ewe
> Rwa rwk Rwa
>
> To check the messages, do you have a regression test in mind?
>

I think we can check with simple error messages. So, basically after
setting a particular value of LC_MESSAGES, execute a query which
returns syntax or any other error, if the error message is the same
irrespective of the locale name returned by _create_locale and
GetLocaleInfoEx, then we should be fine. I want to especially try
where the return value is slightly different by _create_locale and
GetLocaleInfoEx. I know Davinder is trying something like this but
if you can also try then it would be good.

>
>> 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?
>
>
> In Windows wchar_t is 2 bytes, so we would have to do make UTF16 to UFT32 conversions back and forth. Not sure if it is worth the effort.
>

Yeah, I am also not sure about this. So, let us see if anyone else
has any thoughts on this point, otherwise, we can go with wchar
functions as you have in the patch.

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