diff mbox series

libstdc++: Only use std::time_put in std::format for non-C locales

Message ID 20240801213927.388966-1-jwakely@redhat.com
State New
Headers show
Series libstdc++: Only use std::time_put in std::format for non-C locales | expand

Commit Message

Jonathan Wakely Aug. 1, 2024, 9:32 p.m. UTC
Tested x86_64-linux and sparc-solaris.

I really need to add some tests that use modified formats like %EY and
%OS for locales that actually have special forms for those. We don't
have any such tests now, and with this change most of the _M_locale_fmt
code paths are no longer exercised at all. So better tests needed.

-- >8 --

When testing on Solaris I noticed that std/time/year/io.cc was FAILing
because the year 1642 was being formatted as "+(" by %Ey. This turns out
to be because we defer to std::time_put for modified conversion specs,
and std::time_put uses std::strftime, and that's undefined for years
before 1970. In particular, years before 1900 mean that the tm_year
field is negative, which then causes incorrect results from strftime on
at least Solaris and AIX.

I've raised the general problem with LWG, but we can fix the FAILing
test case (and probably improve performance slightly) by ignoring the E
and O modifiers when the formatting locale is the "C" locale. The
modifiers have no effect for the C locale, so we can just treat %Ey as
%y and format it directly. This doesn't fix anything when the formatting
locale isn't the C locale, but that case is not adequately tested, so
doesn't cause any FAIL right now!

The naïve fix would be simply:

  if (__mod)
    if (auto __loc = _M_locale(__ctx); __loc != locale::classic())
      // ...

However when the format string doesn't use the 'L' option, _M_locale
always returns locale::classic(). In that case, we make a copy of the
classic locale (which calls the non-inline copy constructor in
the library), then make another copy of the classic locale, then compare
the two. We can avoid all that by checking for the 'L' option first,
instead of letting _M_locale do that:

  if (__mod && _M_spec._M_localized)
    if (auto __loc = __ctx.locale(); __loc != locale::classic())
      // ...

We could optimize this further if we had a __is_classic(__loc) function
that would do the __loc == locale::classic() check without making any
copies or non-inline calls. That would require examining the locale's
_M_impl member, and probably require checking its name, because the
locale::_S_classic singleton is not exported from the library.

For _M_S the change is slightly different from the other functions,
because if we skip using std::time_put for %OS then we fall through to
the code that potentially prints fractional seconds, but the %OS format
only prints whole seconds. So we need to format whole seconds directly
when not using std::time_put, instead of falling through to the code
below.

libstdc++-v3/ChangeLog:

	* include/bits/chrono_io.h (__formatter_chrono::_M_C_y_Y):
	Ignore modifiers unless the formatting locale is not the C
	locale.
	(__formatter_chrono::_M_d_e): Likewise.
	(__formatter_chrono::_M_H_I): Likewise.
	(__formatter_chrono::_M_m): Likewise.
	(__formatter_chrono::_M_M): Likewise.
	(__formatter_chrono::_M_S): Likewise.
	(__formatter_chrono::_M_u_w): Likewise.
	(__formatter_chrono::_M_U_V_W): Likewise.
---
 libstdc++-v3/include/bits/chrono_io.h | 133 ++++++++++++++------------
 1 file changed, 74 insertions(+), 59 deletions(-)
diff mbox series

Patch

diff --git a/libstdc++-v3/include/bits/chrono_io.h b/libstdc++-v3/include/bits/chrono_io.h
index a449ffdc558..38a0b002c81 100644
--- a/libstdc++-v3/include/bits/chrono_io.h
+++ b/libstdc++-v3/include/bits/chrono_io.h
@@ -917,13 +917,14 @@  namespace __format
 
 	  chrono::year __y = _S_year(__t);
 
-	  if (__mod) [[unlikely]]
-	    {
-	      struct tm __tm{};
-	      __tm.tm_year = (int)__y - 1900;
-	      return _M_locale_fmt(std::move(__out), _M_locale(__ctx), __tm,
-				   __conv, __mod);
-	    }
+	  if (__mod && _M_spec._M_localized) [[unlikely]]
+	    if (auto __loc = __ctx.locale(); __loc != locale::classic())
+	      {
+		struct tm __tm{};
+		__tm.tm_year = (int)__y - 1900;
+		return _M_locale_fmt(std::move(__out), __loc, __tm,
+				     __conv, __mod);
+	      }
 
 	  basic_string<_CharT> __s;
 	  int __yi = (int)__y;
@@ -985,13 +986,14 @@  namespace __format
 	  chrono::day __d = _S_day(__t);
 	  unsigned __i = (unsigned)__d;
 
-	  if (__mod) [[unlikely]]
-	    {
-	      struct tm __tm{};
-	      __tm.tm_mday = __i;
-	      return _M_locale_fmt(std::move(__out), _M_locale(__ctx), __tm,
-				   (char)__conv, 'O');
-	    }
+	  if (__mod && _M_spec._M_localized) [[unlikely]]
+	    if (auto __loc = __ctx.locale(); __loc != locale::classic())
+	      {
+		struct tm __tm{};
+		__tm.tm_mday = __i;
+		return _M_locale_fmt(std::move(__out), __loc, __tm,
+				     (char)__conv, 'O');
+	      }
 
 	  auto __sv = _S_two_digits(__i);
 	  _CharT __buf[2];
@@ -1051,13 +1053,14 @@  namespace __format
 	  const auto __hms = _S_hms(__t);
 	  int __i = __hms.hours().count();
 
-	  if (__mod) [[unlikely]]
-	    {
-	      struct tm __tm{};
-	      __tm.tm_hour = __i;
-	      return _M_locale_fmt(std::move(__out), _M_locale(__ctx), __tm,
-				   (char)__conv, 'O');
-	    }
+	  if (__mod && _M_spec._M_localized) [[unlikely]]
+	    if (auto __loc = __ctx.locale(); __loc != locale::classic())
+	      {
+		struct tm __tm{};
+		__tm.tm_hour = __i;
+		return _M_locale_fmt(std::move(__out), __loc, __tm,
+				     (char)__conv, 'O');
+	      }
 
 	  if (__conv == _CharT('I'))
 	    {
@@ -1109,13 +1112,14 @@  namespace __format
 	  auto __m = _S_month(__t);
 	  auto __i = (unsigned)__m;
 
-	  if (__mod) [[unlikely]] // %Om
-	    {
-	      struct tm __tm{};
-	      __tm.tm_mon = __i - 1;
-	      return _M_locale_fmt(std::move(__out), _M_locale(__ctx), __tm,
-				   'm', 'O');
-	    }
+	  if (__mod && _M_spec._M_localized) [[unlikely]] // %Om
+	    if (auto __loc = __ctx.locale(); __loc != locale::classic())
+	      {
+		struct tm __tm{};
+		__tm.tm_mon = __i - 1;
+		return _M_locale_fmt(std::move(__out), __loc, __tm,
+				     'm', 'O');
+	      }
 
 	  return __format::__write(std::move(__out), _S_two_digits(__i));
 	}
@@ -1131,13 +1135,14 @@  namespace __format
 	  auto __m = _S_hms(__t).minutes();
 	  auto __i = __m.count();
 
-	  if (__mod) [[unlikely]] // %OM
-	    {
-	      struct tm __tm{};
-	      __tm.tm_min = __i;
-	      return _M_locale_fmt(std::move(__out), _M_locale(__ctx), __tm,
-				   'M', 'O');
-	    }
+	  if (__mod && _M_spec._M_localized) [[unlikely]] // %OM
+	    if (auto __loc = __ctx.locale(); __loc != locale::classic())
+	      {
+		struct tm __tm{};
+		__tm.tm_min = __i;
+		return _M_locale_fmt(std::move(__out), __loc, __tm,
+				     'M', 'O');
+	      }
 
 	  return __format::__write(std::move(__out), _S_two_digits(__i));
 	}
@@ -1226,22 +1231,30 @@  namespace __format
 	  // %S  Seconds as a decimal number.
 	  // %OS The locale's alternative representation.
 	  auto __hms = _S_hms(__t);
+	  auto __s = __hms.seconds();
 
 	  if (__mod) [[unlikely]] // %OS
 	    {
-	      struct tm __tm{};
-	      __tm.tm_sec = (int)__hms.seconds().count();
-	      return _M_locale_fmt(std::move(__out), _M_locale(__ctx), __tm,
-				   'S', 'O');
+	      if (_M_spec._M_localized)
+		if (auto __loc = __ctx.locale(); __loc != locale::classic())
+		  {
+		    struct tm __tm{};
+		    __tm.tm_sec = (int)__s.count();
+		    return _M_locale_fmt(std::move(__out), __loc, __tm,
+					 'S', 'O');
+		  }
+
+	      // %OS formats don't include subseconds, so just format that:
+	      return __format::__write(std::move(__out),
+				       _S_two_digits(__s.count()));
 	    }
 
 	  if constexpr (__hms.fractional_width == 0)
 	    __out = __format::__write(std::move(__out),
-				      _S_two_digits(__hms.seconds().count()));
+				      _S_two_digits(__s.count()));
 	  else
 	    {
 	      locale __loc = _M_locale(__ctx);
-	      auto __s = __hms.seconds();
 	      auto __ss = __hms.subseconds();
 	      using rep = typename decltype(__ss)::rep;
 	      if constexpr (is_floating_point_v<rep>)
@@ -1291,13 +1304,14 @@  namespace __format
 
 	  chrono::weekday __wd = _S_weekday(__t);
 
-	  if (__mod) [[unlikely]]
-	    {
-	      struct tm __tm{};
-	      __tm.tm_wday = __wd.c_encoding();
-	      return _M_locale_fmt(std::move(__out), _M_locale(__ctx), __tm,
-				   (char)__conv, 'O');
-	    }
+	  if (__mod && _M_spec._M_localized) [[unlikely]]
+	    if (auto __loc = __ctx.locale(); __loc != locale::classic())
+	      {
+		struct tm __tm{};
+		__tm.tm_wday = __wd.c_encoding();
+		return _M_locale_fmt(std::move(__out), __loc, __tm,
+				     (char)__conv, 'O');
+	      }
 
 	  unsigned __wdi = __conv == 'u' ? __wd.iso_encoding()
 					 : __wd.c_encoding();
@@ -1320,17 +1334,18 @@  namespace __format
 	  auto __d = _S_days(__t);
 	  using _TDays = decltype(__d); // Either sys_days or local_days.
 
-	  if (__mod) [[unlikely]]
-	    {
-	      const year_month_day __ymd(__d);
-	      const year __y = __ymd.year();
-	      struct tm __tm{};
-	      __tm.tm_year = (int)__y - 1900;
-	      __tm.tm_yday = (__d - _TDays(__y/January/1)).count();
-	      __tm.tm_wday = weekday(__d).c_encoding();
-	      return _M_locale_fmt(std::move(__out), _M_locale(__ctx), __tm,
-				   (char)__conv, 'O');
-	    }
+	  if (__mod && _M_spec._M_localized) [[unlikely]]
+	    if (auto __loc = __ctx.locale(); __loc != locale::classic())
+	      {
+		const year_month_day __ymd(__d);
+		const year __y = __ymd.year();
+		struct tm __tm{};
+		__tm.tm_year = (int)__y - 1900;
+		__tm.tm_yday = (__d - _TDays(__y/January/1)).count();
+		__tm.tm_wday = weekday(__d).c_encoding();
+		return _M_locale_fmt(std::move(__out), __loc, __tm,
+				     (char)__conv, 'O');
+	      }
 
 	  _TDays __first; // First day of week 1.
 	  if (__conv == 'V') // W01 begins on Monday before first Thursday.