Thread: Make uselocale protection more consistent

Make uselocale protection more consistent

From
"Tristan Partin"
Date:
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]: https://www.postgresql.org/message-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi

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

Attachment

Re: Make uselocale protection more consistent

From
Peter Eisentraut
Date:
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]: https://www.postgresql.org/message-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.




Re: Make uselocale protection more consistent

From
Thomas Munro
Date:
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]: https://www.postgresql.org/message-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]
https://www.postgresql.org/message-id/flat/CA%2BhUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw%40mail.gmail.com#aef2f2274b28ff8a36f9b8a598e3cec0



Re: Make uselocale protection more consistent

From
"Tristan Partin"
Date:
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]: https://www.postgresql.org/message-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]
https://www.postgresql.org/message-id/flat/CA%2BhUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw%40mail.gmail.com#aef2f2274b28ff8a36f9b8a598e3cec0

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)



Re: Make uselocale protection more consistent

From
Peter Eisentraut
Date:
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]
https://www.postgresql.org/message-id/flat/CA%2BhUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw%40mail.gmail.com#aef2f2274b28ff8a36f9b8a598e3cec0

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




Re: Make uselocale protection more consistent

From
Peter Eisentraut
Date:
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]https://www.postgresql.org/message-id/flat/CA%2BhUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw%40mail.gmail.com#aef2f2274b28ff8a36f9b8a598e3cec0
> 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




Re: Make uselocale protection more consistent

From
"Tristan Partin"
Date:
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]https://www.postgresql.org/message-id/flat/CA%2BhUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw%40mail.gmail.com#aef2f2274b28ff8a36f9b8a598e3cec0
> > 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)