On Fri, 5 May 2023, Jonathan Wakely wrote:
>
>
> On Fri, 5 May 2023 at 10:43, Florian Weimer wrote:
> * Jonathan Wakely via Libstdc:
>
> > We could use strtod for a single-threaded target (i.e.
> > !defined(_GLIBCXX_HAS_GTHREADS) by changing the global locale using
> > setlocale, instead of changing the per-thread locale using uselocale.
>
> This is not generally safe because the call to setlocale is still
> observable to applications in principle because a previous pointer
> returned from setlocale they have store could be invalidated.
>
>
> Ah yes, good point, thanks. I think that's a non-starter then. I still think
> using RAII makes the from_chars_impl function easier to read, so here's a
> version of that patch without the single-threaded
> conditions.
>
> commit 4dc5b8864ec527e699d35880fbc706157113f92b
> Author: Jonathan Wakely <jwak...@redhat.com>
> Date: Thu May 4 15:22:07 2023
>
> libstdc++: Use RAII types in strtod-based std::from_chars implementation
>
> This adds auto_locale and auto_ferounding types to use RAII for changing
> and restoring the local and floating-point environment when using strtod
> to implement std::from_chars.
>
> The destructors for the RAII objects run slightly later than the
> previous statements that restored the locale/fenv, but the differences
> are just some trivial assignments and an isinf call.
>
> libstdc++-v3/ChangeLog:
>
> * src/c++17/floating_from_chars.cc [USE_STRTOD_FOR_FROM_CHARS]
> (auto_locale, auto_ferounding): New class types.
> (from_chars_impl): Use auto_locale and auto_ferounding.
>
> diff --git a/libstdc++-v3/src/c++17/floating_from_chars.cc
> b/libstdc++-v3/src/c++17/floating_from_chars.cc
> index 78b9d92cdc0..7b3bdf445e3 100644
> --- a/libstdc++-v3/src/c++17/floating_from_chars.cc
> +++ b/libstdc++-v3/src/c++17/floating_from_chars.cc
> @@ -597,6 +597,69 @@ namespace
> return buf.c_str();
> }
>
> + // RAII type to change and restore the locale.
> + struct auto_locale
> + {
> +#if _GLIBCXX_HAVE_USELOCALE
> + // When we have uselocale we can change the current thread's locale.
> + locale_t loc;
> + locale_t orig;
It's not a big deal, but we could consider making these members const
too, like in auto_ferounding.
LGTM. I noticed sprintf_ld from floating_to_chars.cc could benefit from
auto_ferounding as well.
> +
> + auto_locale()
> + : loc(::newlocale(LC_ALL_MASK, "C", (locale_t)0))
> + {
> + if (loc)
> + orig = ::uselocale(loc);
> + else
> + ec = errc{errno};
> + }
> +
> + ~auto_locale()
> + {
> + if (loc)
> + {
> + ::uselocale(orig);
> + ::freelocale(loc);
> + }
> + }
> +#else
> + // Otherwise, we can't change the locale and so strtod can't be used.
> + auto_locale() = delete;
> +#endif
> +
> + explicit operator bool() const noexcept { return ec == errc{}; }
> +
> + errc ec{};
> +
> + auto_locale(const auto_locale&) = delete;
> + auto_locale& operator=(const auto_locale&) = delete;
> + };
> +
> + // RAII type to change and restore the floating-point environment.
> + struct auto_ferounding
> + {
> +#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST)
> + const int rounding = std::fegetround();
> +
> + auto_ferounding()
> + {
> + if (rounding != FE_TONEAREST)
> + std::fesetround(FE_TONEAREST);
> + }
> +
> + ~auto_ferounding()
> + {
> + if (rounding != FE_TONEAREST)
> + std::fesetround(rounding);
> + }
> +#else
> + auto_ferounding() = default;
> +#endif
> +
> + auto_ferounding(const auto_ferounding&) = delete;
> + auto_ferounding& operator=(const auto_ferounding&) = delete;
> + };
> +
> // Convert the NTBS `str` to a floating-point value of type `T`.
> // If `str` cannot be converted, `value` is unchanged and `0` is returned.
> // Otherwise, let N be the number of characters consumed from `str`.
> @@ -607,16 +670,11 @@ namespace
> ptrdiff_t
> from_chars_impl(const char* str, T& value, errc& ec) noexcept
> {
> - if (locale_t loc = ::newlocale(LC_ALL_MASK, "C", (locale_t)0)) [[likely]]
> + auto_locale loc;
> +
> + if (loc)
> {
> - locale_t orig = ::uselocale(loc);
> -
> -#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST)
> - const int rounding = std::fegetround();
> - if (rounding != FE_TONEAREST)
> - std::fesetround(FE_TONEAREST);
> -#endif
> -
> + auto_ferounding rounding;
> const int save_errno = errno;
> errno = 0;
> char* endptr;
> @@ -647,14 +705,6 @@ namespace
> #endif
> const int conv_errno = std::__exchange(errno, save_errno);
>
> -#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST)
> - if (rounding != FE_TONEAREST)
> - std::fesetround(rounding);
> -#endif
> -
> - ::uselocale(orig);
> - ::freelocale(loc);
> -
> const ptrdiff_t n = endptr - str;
> if (conv_errno == ERANGE) [[unlikely]]
> {
> @@ -675,8 +725,8 @@ namespace
> }
> return n;
> }
> - else if (errno == ENOMEM)
> - ec = errc::not_enough_memory;
> + else
> + ec = loc.ec;
>
> return 0;
> }
