[committed] libstdc++: Fix std::format for floating-point chrono::time_point [PR113500]

[committed] libstdc++: Fix std::format for floating-point chrono::time_point [PR113500]

[Jonathan Wakely]

Tested aarch64-linux. Pushed to trunk.

Backport to gcc-13 would be good too, I think.

-- >8 --

Currently trying to use std::format with certain specializations of
std::chrono::time_point is ill-formed, due to one member function of the
__formatter_chrono type which tries to write a time_point to an ostream.
For sys_time<floating-point> or sys_time with a period greater than days
there is no operator<< that can be used.

That operator<< is only needed when using an empty chrono-specs in the
format string, like "{}", but the ill-formed expression gives an error
even if not actually used. This means it's not possible to format some
other specializations of chrono::time_point, even when using a non-empty
chrono-specs.

This fixes it by avoiding using 'os << t' for all chrono::time_point
specializations, and instead using std::format("{:L%F %T}", t). So that
we continue to reject std::format("{}", sys_time{1.0s}) a check for
empty chrono-specs is added to the formatter<sys_time<D>, C>
specialization.

While testing this I noticed that the output for %S with a
floating-point duration was incorrect, as the subseconds part was being
appended to the seconds without a decimal point, and without the correct
number of leading zeros.

libstdc++-v3/ChangeLog:

	PR libstdc++/113500
	* include/bits/chrono_io.h (__formatter_chrono::_M_S): Fix
	printing of subseconds with floating-point rep.
	(__formatter_chrono::_M_format_to_ostream): Do not write
	time_point specializations directly to the ostream.
	(formatter<chrono::sys_time<D>, C>::parse): Do not allow an
	empty chrono-spec if the type fails to meet the constraints for
	writing to an ostream with operator<<.
	* testsuite/std/time/clock/file/io.cc: Check formatting
	non-integral times with empty chrono-specs.
	* testsuite/std/time/clock/gps/io.cc: Likewise.
	* testsuite/std/time/clock/utc/io.cc: Likewise.
	* testsuite/std/time/hh_mm_ss/io.cc: Likewise.
---
 libstdc++-v3/include/bits/chrono_io.h         | 71 +++++++++++++------
 .../testsuite/std/time/clock/file/io.cc       | 17 +++++
 .../testsuite/std/time/clock/gps/io.cc        | 27 +++++++
 .../testsuite/std/time/clock/utc/io.cc        |  4 ++
 .../testsuite/std/time/hh_mm_ss/io.cc         | 28 +++++++-
 5 files changed, 124 insertions(+), 23 deletions(-)

diff --git a/libstdc++-v3/include/bits/chrono_io.h b/libstdc++-v3/include/bits/chrono_io.h
index 7b5876b24e6..82f2d39ec44 100644
--- a/libstdc++-v3/include/bits/chrono_io.h
+++ b/libstdc++-v3/include/bits/chrono_io.h
@@ -681,6 +681,7 @@ namespace __format
 	    return __fc.locale();
 	}
 
+      // Format for empty chrono-specs, e.g. "{}" (C++20 [time.format] p6).
       // TODO: consider moving body of every operator<< into this function
       // and use std::format("{}", t) to implement those operators. That
       // would avoid std::format("{}", t) calling operator<< which calls
@@ -702,6 +703,22 @@ namespace __format
 
 	      if constexpr (__is_specialization_of<_Tp, __utc_leap_second>)
 		__os << __t._M_date << ' ' << __t._M_time;
+	      else if constexpr (chrono::__is_time_point_v<_Tp>)
+		{
+		  // Need to be careful here because not all specializations
+		  // of chrono::sys_time can be written to an ostream.
+		  // For the specializations of time_point that can be
+		  // formatted with an empty chrono-specs, either it's a
+		  // sys_time with period greater or equal to days:
+		  if constexpr (is_convertible_v<_Tp, chrono::sys_days>)
+		    __os << _S_date(__t);
+		  else // Or it's formatted as "{:L%F %T}":
+		    {
+		      auto __days = chrono::floor<chrono::days>(__t);
+		      __os << chrono::year_month_day(__days) << ' '
+			 << chrono::hh_mm_ss(__t - __days);
+		    }
+		}
 	      else
 		{
 		  if constexpr (chrono::__is_duration_v<_Tp>)
@@ -1122,39 +1139,43 @@ namespace __format
 				   'S', 'O');
 	    }
 
-	  __out = __format::__write(std::move(__out),
-				    _S_two_digits(__hms.seconds().count()));
-	  if constexpr (__hms.fractional_width != 0)
+	  if constexpr (__hms.fractional_width == 0)
+	    __out = __format::__write(std::move(__out),
+				      _S_two_digits(__hms.seconds().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>)
 		{
+		  chrono::duration<rep> __fs = __s + __ss;
 		  __out = std::format_to(std::move(__out), __loc,
-					 _GLIBCXX_WIDEN("{:.{}Lg}"),
-					 __ss.count(),
-					 __hms.fractional_width);
-		}
-	      else if constexpr (is_integral_v<rep>)
-		{
-		  const auto& __np
-		    = use_facet<numpunct<_CharT>>(__loc);
-		  __out = std::format_to(std::move(__out),
-					 _GLIBCXX_WIDEN("{}{:0{}}"),
-					 __np.decimal_point(),
-					 __ss.count(),
+					 _GLIBCXX_WIDEN("{:#0{}.{}Lf}"),
+					 __fs.count(),
+					 3 + __hms.fractional_width,
 					 __hms.fractional_width);
 		}
 	      else
 		{
 		  const auto& __np
 		    = use_facet<numpunct<_CharT>>(__loc);
+		  __out = __format::__write(std::move(__out),
+					    _S_two_digits(__s.count()));
 		  *__out++ = __np.decimal_point();
-		  auto __str = std::format(_S_empty_spec, __ss.count());
-		  __out = std::format_to(_GLIBCXX_WIDEN("{:0>{}s}"),
-							__str,
-							__hms.fractional_width);
+		  if constexpr (is_integral_v<rep>)
+		    __out = std::format_to(std::move(__out),
+					   _GLIBCXX_WIDEN("{:0{}}"),
+					   __ss.count(),
+					   __hms.fractional_width);
+		  else
+		    {
+		      auto __str = std::format(_S_empty_spec, __ss.count());
+		      __out = std::format_to(_GLIBCXX_WIDEN("{:0>{}s}"),
+					     __str,
+					     __hms.fractional_width);
+		    }
 		}
 	    }
 	  return __out;
@@ -1911,7 +1932,13 @@ namespace __format
       template<typename _ParseContext>
 	constexpr typename _ParseContext::iterator
 	parse(_ParseContext& __pc)
-	{ return _M_f._M_parse(__pc, __format::_ZonedDateTime); }
+	{
+	  auto __next = _M_f._M_parse(__pc, __format::_ZonedDateTime);
+	  if constexpr (!__stream_insertable)
+	    if (_M_f._M_spec._M_chrono_specs.empty())
+	      __format::__invalid_chrono_spec(); // chrono-specs can't be empty
+	  return __next;
+	}
 
       template<typename _FormatContext>
 	typename _FormatContext::iterator
@@ -1920,6 +1947,10 @@ namespace __format
 	{ return _M_f._M_format(__t, __fc); }
 
     private:
+      static constexpr bool __stream_insertable
+	= requires (basic_ostream<_CharT>& __os,
+		    chrono::sys_time<_Duration> __t) { __os << __t; };
+
       __format::__formatter_chrono<_CharT> _M_f;
     };
 
diff --git a/libstdc++-v3/testsuite/std/time/clock/file/io.cc b/libstdc++-v3/testsuite/std/time/clock/file/io.cc
index 39eb0dbcf51..3eee6bf5cc2 100644
--- a/libstdc++-v3/testsuite/std/time/clock/file/io.cc
+++ b/libstdc++-v3/testsuite/std/time/clock/file/io.cc
@@ -17,6 +17,23 @@ test_ostream()
   VERIFY( ss1.str() == ss2.str() );
 }
 
+void
+test_format()
+{
+  using namespace std::chrono;
+  auto t = file_clock::now();
+
+  auto s = std::format("{}", t);
+  std::ostringstream ss;
+  ss << t;
+  VERIFY( s == ss.str() );
+
+  // PR libstdc++/113500
+  auto ft = clock_cast<file_clock>(sys_days(2024y/January/21)) + 0ms + 2.5s;
+  s = std::format("{}", ft);
+  VERIFY( s == "2024-01-17 00:00:02.500");
+}
+
 void
 test_parse()
 {
diff --git a/libstdc++-v3/testsuite/std/time/clock/gps/io.cc b/libstdc++-v3/testsuite/std/time/clock/gps/io.cc
index 6f4544fcd97..5d15713c69d 100644
--- a/libstdc++-v3/testsuite/std/time/clock/gps/io.cc
+++ b/libstdc++-v3/testsuite/std/time/clock/gps/io.cc
@@ -3,6 +3,7 @@
 
 #include <chrono>
 #include <format>
+#include <sstream>
 #include <testsuite_hooks.h>
 
 void
@@ -16,6 +17,32 @@ test_ostream()
 
   auto s = format("{0:%F %T %Z} == {1:%F %T %Z}", st, gt);
   VERIFY( s == "2000-01-01 00:00:00 UTC == 2000-01-01 00:00:13 GPS" );
+
+  std::ostringstream ss;
+  ss << gt;
+  VERIFY( ss.str() == "2000-01-01 00:00:13" );
+
+  gps_time<duration<float>> gtf = gt;
+  ss.str("");
+  ss.clear();
+  ss << (gps_time<duration<long double>>(gt) + 20ms);
+  VERIFY( ss.str() == "2000-01-01 00:00:13.020" );
+}
+
+void
+test_format()
+{
+  using std::format;
+  using namespace std::chrono;
+
+  auto st = sys_days{2000y/January/1};
+  auto gt = clock_cast<gps_clock>(st);
+  auto s = std::format("{}", gt);
+  VERIFY( s == "2000-01-01 00:00:13" );
+
+  // PR libstdc++/113500
+  s = std::format("{}", gt + 150ms + 10.5s);
+  VERIFY( s == "2000-01-01 00:00:35.650" );
 }
 
 void
diff --git a/libstdc++-v3/testsuite/std/time/clock/utc/io.cc b/libstdc++-v3/testsuite/std/time/clock/utc/io.cc
index 58e358f3dbf..55c53dc4057 100644
--- a/libstdc++-v3/testsuite/std/time/clock/utc/io.cc
+++ b/libstdc++-v3/testsuite/std/time/clock/utc/io.cc
@@ -112,6 +112,10 @@ test_format()
 
   s = std::format("{:%T}", leap + 1s);
   VERIFY( s == "00:00:00" );
+
+  // PR libstdc++/113500
+  s = std::format("{}", leap + 100ms + 2.5s);
+  VERIFY( s == "2017-01-01 00:00:01.600");
 }
 
 void
diff --git a/libstdc++-v3/testsuite/std/time/hh_mm_ss/io.cc b/libstdc++-v3/testsuite/std/time/hh_mm_ss/io.cc
index 8eb80422e6e..d651dfbcdc8 100644
--- a/libstdc++-v3/testsuite/std/time/hh_mm_ss/io.cc
+++ b/libstdc++-v3/testsuite/std/time/hh_mm_ss/io.cc
@@ -35,9 +35,31 @@ test_ostream()
     VERIFY( out.str() == "18:15:45.123" );
   }
 
-  ostringstream out;
-  out << hh_mm_ss{65745s};
-  VERIFY( out.str() == "18:15:45" );
+  {
+    ostringstream out;
+    out << hh_mm_ss{65745s};
+    VERIFY( out.str() == "18:15:45" );
+  }
+
+  {
+    ostringstream out;
+    out << hh_mm_ss{0.020s};
+    // hh_mm_ss<duration<long double>>::fractional_width == 0 so no subseconds:
+    VERIFY( out.str() == "00:00:00" );
+  }
+
+  {
+    ostringstream out;
+    out << hh_mm_ss<std::chrono::duration<long double, std::nano>>{0.020s};
+    // hh_mm_ss<duration<long double, nano>>::fractional_width == 9:
+    VERIFY( out.str() == "00:00:00.020000000" );
+  }
+
+  {
+    ostringstream out;
+    out << hh_mm_ss{65745s + 20ms};
+    VERIFY( out.str() == "18:15:45.020" );
+  }
 }
 
 void
-- 
2.43.0

Re: [committed] libstdc++: Fix std::format for floating-point chrono::time_point [PR113500]

[Jonathan Wakely]

On Sun, 21 Jan 2024 at 22:27, Jonathan Wakely wrote:
> --- a/libstdc++-v3/testsuite/std/time/clock/file/io.cc
> +++ b/libstdc++-v3/testsuite/std/time/clock/file/io.cc
> @@ -17,6 +17,23 @@ test_ostream()
>    VERIFY( ss1.str() == ss2.str() );
>  }
>
> +void
> +test_format()
> +{
> +  using namespace std::chrono;
> +  auto t = file_clock::now();
> +
> +  auto s = std::format("{}", t);
> +  std::ostringstream ss;
> +  ss << t;
> +  VERIFY( s == ss.str() );
> +
> +  // PR libstdc++/113500
> +  auto ft = clock_cast<file_clock>(sys_days(2024y/January/21)) + 0ms + 2.5s;
> +  s = std::format("{}", ft);
> +  VERIFY( s == "2024-01-17 00:00:02.500");

Well obviously that should be 2024-01-21 and I should add a call to
test_format() in main() so it actually runs.

I'll push this fix:

--- a/libstdc++-v3/testsuite/std/time/clock/file/io.cc
+++ b/libstdc++-v3/testsuite/std/time/clock/file/io.cc
@@ -31,7 +31,7 @@ test_format()
  // PR libstdc++/113500
  auto ft = clock_cast<file_clock>(sys_days(2024y/January/21)) + 0ms + 2.5s;
  s = std::format("{}", ft);
-  VERIFY( s == "2024-01-17 00:00:02.500");
+  VERIFY( s == "2024-01-21 00:00:02.500");
}

void
@@ -54,5 +54,6 @@ test_parse()
int main()
{
  test_ostream();
+  test_format();
  test_parse();
}

Re: [committed] libstdc++: Fix std::format for floating-point chrono::time_point [PR113500]

[Jonathan Wakely]

On Mon, 22 Jan 2024 at 09:51, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On Sun, 21 Jan 2024 at 22:27, Jonathan Wakely wrote:
> > --- a/libstdc++-v3/testsuite/std/time/clock/file/io.cc
> > +++ b/libstdc++-v3/testsuite/std/time/clock/file/io.cc
> > @@ -17,6 +17,23 @@ test_ostream()
> >    VERIFY( ss1.str() == ss2.str() );
> >  }
> >
> > +void
> > +test_format()
> > +{
> > +  using namespace std::chrono;
> > +  auto t = file_clock::now();
> > +
> > +  auto s = std::format("{}", t);
> > +  std::ostringstream ss;
> > +  ss << t;
> > +  VERIFY( s == ss.str() );
> > +
> > +  // PR libstdc++/113500
> > +  auto ft = clock_cast<file_clock>(sys_days(2024y/January/21)) + 0ms + 2.5s;
> > +  s = std::format("{}", ft);
> > +  VERIFY( s == "2024-01-17 00:00:02.500");
>
> Well obviously that should be 2024-01-21 and I should add a call to
> test_format() in main() so it actually runs.

And I did the same in the gps/io.cc test!

I'll push this fix for that one:

--- a/libstdc++-v3/testsuite/std/time/clock/gps/io.cc
+++ b/libstdc++-v3/testsuite/std/time/clock/gps/io.cc
@@ -42,7 +42,7 @@ test_format()

  // PR libstdc++/113500
  s = std::format("{}", gt + 150ms + 10.5s);
-  VERIFY( s == "2000-01-01 00:00:35.650" );
+  VERIFY( s == "2000-01-01 00:00:23.650" );
}

void
@@ -65,5 +65,6 @@ test_parse()
int main()
{
  test_ostream();
+  test_format();
  test_parse();
}