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

Thanks for the review comments.

On Tue, Apr 14, 2020 at 9:12 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:

> >>I m still working on testing this patch. If anyone has Idea please
> suggest.
> I still see problems with this patch.
>
> 1. Variable loct have redundant initialization, it would be enough to
> declare so: _locale_t loct;
> 2. Style white space in variable rc declaration.
> 3. Style variable cp_index can be reduced.
> if (tmp != NULL) {
> size_t cp_index;
>
> cp_index = (size_t)(tmp - winlocname);
> strncpy(loc_name, winlocname, cp_index);
> loc_name[cp_index] = '\0';
> 4. Memory leak if _WIN32_WINNT >= 0x0600 is true, _free_locale(loct); is
> not called.
>
I resolved the above comments.

> 5. Why call _create_locale if _WIN32_WINNT >= 0x0600 is true and loct is
> not used?
>
_create_locale can take bigger input than GetLocaleInfoEx. But we are
interested in
*language[_country-region[.code-page]]*. We are using _create_locale to
validate
the given input. The reason is we can't verify the locale name if it is
appended with
code-page by using GetLocaleInfoEx. So before parsing, we verify if the
whole input
locale name is valid by using _create_locale. I hope that answers your
question.

I have attached the patch.
--
Regards,
Davinder
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 14, 2020 at 1:07 PM davinder singh <davindersingh2692(at)gmail(dot)com>
wrote:

>
> On Fri, Apr 10, 2020 at 5:33 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> >
> > It seems the right direction to use GetLocaleInfoEx as we have already
> > used it to handle a similar problem (lc_codepage is missing in
> > _locale_t in higher versions of MSVC (cf commit 0fb54de9aa)) in
> > chklocale.c. However, I see that we have added a manual parsing there
> > if GetLocaleInfoEx doesn't parse it. I think that addresses your
> > concern for _create_locale handling bigger sets. Don't we need
> > something equivalent here for the cases which GetLocaleInfoEx doesn't
> > support?
> I am in investigating in similar lines, I think the following explanation
> can help.
> From Microsoft doc.
> The locale argument to the setlocale, _wsetlocale, _create_locale, and
> _wcreate_locale is
> locale :: "locale-name"
> | *"language[_country-region[.code-page]]"*
> | ".code-page"
> | "C"
> | ""
> | NULL
>
> For GetLocaleInfoEx locale argument is
> *<language>-<REGION>*
> <language>-<Script>-<REGION>
>
> As we can see _create_locale will also accept the locale appended with
> code-page but that is not the case in lateral.
> The important point is in our current issue we need locale name only and
> both
> functions(GetLocaleInfoEx, _create_locale) support an equal number of
> locales
> names if go by the syntax of the locale described on Microsoft documents.
> With that thought, I am parsing the input
> string to remove the code-page and give it as input to GelLocaleInfoEx.
> I have attached the new patch.
>
> > How have you ensured the testing of this code? I see that we have
> > src\test\locale in our test directory. Can we test using that?
> I don't know how to use these tests on windows, but I had a look in these
> tests, I didn't found any test which could hit the function we are
> modifying.
> I m still working on testing this patch. If anyone has Idea please suggest.
>
> [1] https://docs.microsoft.com/en-us/windows/win32/intl/locale-names
> [2]
> https://docs.microsoft.com/en-us/windows/win32/api/winnls/nf-winnls-getlocaleinfoex
> [3]
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/create-locale-wcreate-locale?view=vs-2019
> --
> Regards,
> Davinder
> EnterpriseDB: http://www.enterprisedb.com
>

--
Regards,
Davinder
EnterpriseDB: http://www.enterprisedb.com