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.
