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

On 19.08.24 08:29, Thomas Munro wrote:
> Here is a slightly better version of patch 0003.  I removed some
> unnecessary refactoring, making the patch smaller.
> 
> FTR I wrote a small program[1] for CI to test the assumptions about
> Windows in 0001.  I printed out the addresses of the objects, to
> confirm that different threads were looking at different objects once
> the thread local mode was activated, and also assert that the struct
> contents were as expected while 8 threads switched locales in a tight
> loop, and the output[2] looked OK to me.
> 
> [1] https://github.com/macdice/hello-windows/blob/793eb2fe3e6738c200781f681a22a7e6358f39e5/test.c
> [2] https://cirrus-ci.com/task/4650412253380608

Review of the patch 
v5-0002-Use-thread-safe-strftime_l-instead-of-strftime.patch:

This all looks very sensible.  My only comment on the code is that for 
handling error returns from newlocale() and _create_locale() we already 
have report_newlocale_failure(), which handles various special cases. 
(But it doesn't do the _dosmaperr() that your patch does.)  It would be 
best if you used that as well (and maybe improve as needed).

A couple of comments on the commit message:

 > Use thread-safe strftime_l() instead of strftime().

I don't think this is about thread-safety of either function?  It's more 
that the latter requires a non-thread-safe code structure around it.  I 
would frame this more around the use-locale_t-everywhere theme than the 
thread-safety theme.

 > While here, adjust error message for strftime_l() failure: it does not
 > set errno, so no %m.

Although POSIX says that strftime() and strftime_l() should change 
errno, experimentation shows that they do not.  So this is fine.  But I 
thought also that the previous code was problematic because errno could 
be overwritten since the failing call, so you wouldn't get a very 
accurate error message anyway.