On Thu, 11 May 2023 at 17:04, Patrick Palka <ppa...@redhat.com> wrote:
> 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.
>
Done for loc, but not for orig (which is currently init'd in the ctor body).
>
> LGTM. I noticed sprintf_ld from floating_to_chars.cc could benefit from
> auto_ferounding as well.
>
Ah yes. Maybe we should share the class, so we don't have two different
types with internal linkage, and two RTTI definitions etc.
For now I'll just push this patch, and make a note to reuse auto_ferounding
in the other file later.
Thanks for the review.
>
> > +
> > + 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;
> > }
>
>
