Thread: Make uselocale protection more consistent
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
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.
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
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)
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.
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
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)