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.
