On 11.08.24 00:11, Thomas Munro wrote:
> v4 adds error handling, in case newlocale("C") fails. I created CF
> entry #5166 for this.
I took a look at this. It was quite a complicated discussion that led
to this, but I agree with the solution that was arrived at.
I suggest that the simplification of the xlocale.h configure tests could
be committed separately. This would also be useful independent of this,
and it's a sizeable chunk of this patch.
Also, you're removing the configure test for _configthreadlocale().
Presumably because you're removing all the uses. But wouldn't we need
that back later in the backend maybe? Or is that test even relevant
anymore, that is, are there Windows versions that don't have it?
Adding global includes to port.h doesn't seem great. That's not a place
one would normally look. We already include <locale.h> in c.h anyway,
so it would probably be even better overall if you just added a
conditional #include <xlocale.h> to c.h as well.
For Windows, we already have things like
#define strcoll_l _strcoll_l
in src/include/port/win32_port.h, so it would seem more sensible to add
strtod_l to that list, instead of in port.h.
The error handling with pg_ensure_c_locale(), that's the sort of thing
I'm afraid will be hard to test or even know how it will behave. And it
creates this weird coupling between pgtypeslib and ecpglib that you
mentioned earlier. And if there are other users of PG_C_LOCALE in the
future, there will be similar questions about the proper initialization
and error handling sequence.
I would consider instead making a local static variable in each function
that needs this. For example, numericvar_to_double() might do
{
static locale_t c_locale;
if (!c_locale)
{
c_locale = pg_get_c_locale();
if (!c_locale)
return -1; /* local error reporting convention */
}
...
}
This is a bit more code in total, but then you only initialize what you
need and you can handle errors locally.
