Make uselocale protection more consistent

On 27.06.23 17:02, Tristan Partin wrote:

This is a patch which implements an issue discussed in bug #17946[0]. It
doesn't fix the overarching issue of the bug, but merely a consistency
issue which was found while analyzing code by Heikki. I had originally
submitted the patch within that thread, but for visibility and the
purposes of the commitfest, I have re-sent it in its own thread.

[0]: /messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi

I notice that HAVE_USELOCALE was introduced much later than
HAVE_LOCALE_T, and at the time the code was already using uselocale(),
so perhaps the introduction of HAVE_USELOCALE was unnecessary and should
be reverted.

I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.

On Mon, Jul 3, 2023 at 6:13 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 27.06.23 17:02, Tristan Partin wrote:

This is a patch which implements an issue discussed in bug #17946[0]. It
doesn't fix the overarching issue of the bug, but merely a consistency
issue which was found while analyzing code by Heikki. I had originally
submitted the patch within that thread, but for visibility and the
purposes of the commitfest, I have re-sent it in its own thread.

[0]: /messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi

I notice that HAVE_USELOCALE was introduced much later than
HAVE_LOCALE_T, and at the time the code was already using uselocale(),
so perhaps the introduction of HAVE_USELOCALE was unnecessary and should
be reverted.

I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.

I propose[1]/messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().

[1]: /messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com

On Mon Jul 3, 2023 at 1:24 AM CDT, Thomas Munro wrote:

On Mon, Jul 3, 2023 at 6:13 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 27.06.23 17:02, Tristan Partin wrote:

This is a patch which implements an issue discussed in bug #17946[0]. It
doesn't fix the overarching issue of the bug, but merely a consistency
issue which was found while analyzing code by Heikki. I had originally
submitted the patch within that thread, but for visibility and the
purposes of the commitfest, I have re-sent it in its own thread.

[0]: /messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi

I notice that HAVE_USELOCALE was introduced much later than
HAVE_LOCALE_T, and at the time the code was already using uselocale(),
so perhaps the introduction of HAVE_USELOCALE was unnecessary and should
be reverted.

I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.

I propose[1] that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().

[1] /messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com

I think keeping HAVE_USELOCALE is important for the Windows case as
mentioned. I need it for my localization work where I am ripping out
setlocale() on non-Windows.

--
Tristan Partin
Neon (https://neon.tech)

On 03.07.23 08:24, Thomas Munro wrote:

I propose[1] that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().

[1] /messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com

Ok, it appears your patch is imminent, so let's wait for that and see if
this patch here is still required afterwards.

On 03.07.23 15:21, Tristan Partin wrote:

I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.

I propose[1] that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().

[1]/messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com

I think keeping HAVE_USELOCALE is important for the Windows case as
mentioned. I need it for my localization work where I am ripping out
setlocale() on non-Windows.

The current code is structured

#ifdef HAVE_LOCALE_T
#ifdef HAVE_WCSTOMBS_L
wcstombs_l(...);
#else
uselocale(...);
#endif
#else
elog(ERROR);
#endif

If you just replace HAVE_LOCALE_T with HAVE_USELOCALE, then this would
penalize a platform that has wcstombs_l(), but not uselocale(). I think
the correct structure would be

#if defined(HAVE_WCSTOMBS_L)
wcstombs_l(...);
#elif defined(HAVE_USELOCALE)
uselocale(...);
#else
elog(ERROR);
#endif

On Mon Jul 3, 2023 at 9:21 AM CDT, Peter Eisentraut wrote:

On 03.07.23 15:21, Tristan Partin wrote:

I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.

I propose[1] that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().

[1]/messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com

I think keeping HAVE_USELOCALE is important for the Windows case as
mentioned. I need it for my localization work where I am ripping out
setlocale() on non-Windows.

The current code is structured

#ifdef HAVE_LOCALE_T
#ifdef HAVE_WCSTOMBS_L
wcstombs_l(...);
#else
uselocale(...);
#endif
#else
elog(ERROR);
#endif

If you just replace HAVE_LOCALE_T with HAVE_USELOCALE, then this would
penalize a platform that has wcstombs_l(), but not uselocale(). I think
the correct structure would be

#if defined(HAVE_WCSTOMBS_L)
wcstombs_l(...);
#elif defined(HAVE_USELOCALE)
uselocale(...);
#else
elog(ERROR);
#endif

That makes sense to me. I gave it some more thought. Maybe it makes more
sense to just completely drop HAVE_USELOCALE as mentioned, and protect
calls to it with #ifdef WIN32 or whatever the macro is. HAVE_USELOCALE
might be more descriptive, but I don't really care that much either way.

--
Tristan Partin
Neon (https://neon.tech)