Postgres Pro
Downloads
Home   >   mailing lists

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

From davinder singh
Subject Re: PG compilation error with Visual Studio 2015/2017/2019
Date
Msg-id CAHzhFSFowCDeuFG9+CLGeN4j8u_Dk0GCF2scTf=mVQNVRRNzjg@mail.gmail.com
Whole thread Raw
In response to Re: PG compilation error with Visual Studio 2015/2017/2019  (davinder singh <davindersingh2692@gmail.com>)
Responses Re: PG compilation error with Visual Studio 2015/2017/2019  (Ranier Vilela <ranier.vf@gmail.com>)
List pgsql-hackers
Tree view

Thanks for the review comments.

On Tue, Apr 14, 2020 at 9:12 PM Ranier Vilela <ranier.vf@gmail.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

On Tue, Apr 14, 2020 at 1:07 PM davinder singh <davindersingh2692@gmail.com> wrote:

On Fri, Apr 10, 2020 at 5:33 PM Amit Kapila <amit.kapila16@gmail.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.



--
Regards,
Davinder
Attachment

pgsql-hackers by date:

Previous
From: Dilip Kumar
Date:
Subject: Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions
Next
From: Michael Paquier
Date:
Subject: Re: [patch] some PQExpBuffer are not destroyed in pg_dump