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

On Wed, Apr 22, 2020 at 1:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Tue, Apr 21, 2020 at 5:32 PM Juan José Santamaría Flecha
> <juanjo(dot)santamaria(at)gmail(dot)com> wrote:
> >
> > 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.
>

I am glad we could clear that out, sorry because it was on my hand to
prevent.

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

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

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

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

LGTM.

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

Ok, there is substance for a cleanup 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?
>

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.

Regards,

Juan José Santamaría Flecha

>
>