On Wed, Apr 22, 2020 at 1:43 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Tue, Apr 21, 2020 at 5:32 PM Juan José Santamaría Flecha > <juanjo.santama...@gmail.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 > >
