Skip to content

gh-69998: Fix decoding error in locale.nl_langinfo()#124963

Merged
serhiy-storchaka merged 4 commits intopython:mainfrom
serhiy-storchaka:locale-nl_langinfo
Oct 8, 2024
Merged

gh-69998: Fix decoding error in locale.nl_langinfo()#124963
serhiy-storchaka merged 4 commits intopython:mainfrom
serhiy-storchaka:locale-nl_langinfo

Conversation

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Oct 4, 2024

The function now sets temporarily the LC_CTYPE locale to the locale of the category that determines the requested value if the locales are different and the resulting string is non-ASCII.
This temporary change affects other threads.


📚 Documentation preview 📚: https://cpython-previews--124963.org.readthedocs.build/

The function now sets temporarily the LC_CTYPE locale to the locale
of the category that determines the requested value if the locales are
different and the resulting string is non-ASCII.
This temporary change affects other threads.
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I wrote a similar change in the past using _Py_GetLocaleconvNumeric(). It's used for example in Python/formatter_unicode.c to get the decimal point and thousands separator.

@serhiy-storchaka
Copy link
Member Author

Yes, I used your #4174 as an example.

&& change_locale(langinfo_constants[i].category, &oldloc) < 0)
{
return NULL;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't call nl_langinfo() twice anymore? Why changing the code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I now think that it was not needed. The result can only be changed if setlocale() is called with the same category or LC_ALL. And, even if it is not explicitly stated, I think that calls setlocale(..., NULL) are not counted.

On other hand,I want to avoid duplication of the code for conversion of the result because of #124974.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

locale.localeconv() calls localeconv() only once, whereas it changes the LC_CTYPE locale while reading its output. Let's do the same for nl_langinfo(). If the assumption is wrong, we can adjust the code later.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a single call to nl_langinfo().

@serhiy-storchaka serhiy-storchaka merged commit 93b9e6b into python:main Oct 8, 2024
@serhiy-storchaka serhiy-storchaka deleted the locale-nl_langinfo branch October 8, 2024 08:27
efimov-mikhail pushed a commit to efimov-mikhail/cpython that referenced this pull request Oct 9, 2024
…124963)

The function now sets temporarily the LC_CTYPE locale to the locale
of the category that determines the requested value if the locales are
different and the resulting string is non-ASCII.
This temporary change affects other threads.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants