Re: Thread-safe nl_langinfo() and localeconv()

On 09.02.25 15:52, Peter Eisentraut wrote:
> This patch set is still desirable.  Here is a rebased version of the v5
> patches.  I haven't applied any corrections or review comments.

Here is the same patch set with some review comments.

Patch 0001 looks okay to me. I'm just offering some cosmetic
improvements in patch 0004.

Patch 0002 also looks okay, except that the error handling could be
unified with existing code, as I had previously pointed out. Patch 0005
fixes that.

About patch 0003:

I had previously pointed out that the canonicalization might have been
intentional, and that we have canonicalization of ICU locale names. But
we don't have to keep the setlocale()-based locale checking
implementation just for that, I think. (If this was meant to be a real
feature offered by libc, there should be a get_locale_name(locale_t)
function.)

I'm unsure about the correct error handling of _create_locale() on
Windows. Does _dosmaperr(GetLastError()) do anything useful in this
context, or is this just copied from elsewhere? If it's useful, maybe
it should be added to report_newlocale_failure().

Also, maybe we don't need per-category locale checking? Would it not be
enough to check the locale using LC_ALL_MASK? Is there a scenario where
a locale name would work for one category but not another? I think the
old code was just conveniently coded that way so that you only have to
save and restore one locale category. But we wouldn't have to do it
that way anymore if we use newlocale().