From patchwork Thu Sep 26 11:56:10 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 1989826 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=ZRz41xF2; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4XDtsq5bXJz1xt8 for ; Thu, 26 Sep 2024 22:57:18 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D9372385828B for ; Thu, 26 Sep 2024 12:57:14 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 8EA433858D29 for ; Thu, 26 Sep 2024 12:56:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8EA433858D29 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8EA433858D29 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1727355403; cv=none; b=RiZcOABlGfLrtpKsSWcmiUf3iCn3ByAb3XvOm/A0JCAVrtSkdvSxiqK7B1kD1bNae3MK+sF2bDXzaEZ3t814y4Zgdo3jSnVjAjYMuGmi2y0lrzEAiC1ENQtsXJ+7tp5w3GQbq92W3eH0zbLAD/hrNJH/ZJNf/HA5sscbnkk2BA8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1727355403; c=relaxed/simple; bh=06hPhLTE7mDEwKnaWOS9KlQJkapK6Z3zlMJs6fW9wG8=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=q/IRzvUM/mBzf57RBeiUMZK8uPwgKiSW2Jp0YoDzTbY8HwNMdEGoEH7CHGIGUYxKCGSeCbrgMSLWfSEtfU91pc+fg5x+Ak76Or7uDG0JF1HsML0HA3Q3nv4B69k5s5NWIYiAZwjT6gSAE9EviMePs5qT5c23mLJSfO0JbN48eak= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1727355401; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=MBcNRgeeZHIcnl3G1oYXx13OkGPTnVEisqkBKV1s0q0=; b=ZRz41xF2MkJHgTYdJxflvy6yDmO8PbFaP3LFwT9wBPlwlcOW9OMi83pmE3eYc2rJISSB68 SGc3wHL+u8bjgG2Kaw/+ouBbgqjAy5RVlo2k0eaJG3pvErQgt7BwIy8jm48kx6NfTBLgor 31aLc6W0MuFJaKvNnart1wDJvGaklQU= Received: from mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-256-ipkU77WeNDaZ9O4H81F8XQ-1; Thu, 26 Sep 2024 08:56:38 -0400 X-MC-Unique: ipkU77WeNDaZ9O4H81F8XQ-1 Received: from mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (unknown [10.30.177.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 833CF1943CD2; Thu, 26 Sep 2024 12:56:36 +0000 (UTC) Received: from localhost (unknown [10.42.28.136]) by mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 0F9073003DEC; Thu, 26 Sep 2024 12:56:35 +0000 (UTC) From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [PATCH] libstdc++: Fix rounding in chrono::parse Date: Thu, 26 Sep 2024 12:56:10 +0100 Message-ID: <20240926125634.2003507-1-jwakely@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.30.177.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org Does this rounding heuristic seem reasonable? I have discussed it with Howard and he agreed that rounding "2024-09-22 18:34:56" up to the next day, 2024-09-23, could be surprising. But he convinced me that using chrono::round was correct in general (certainly better than what we have now using chrono::time_point_cast). A period like ratio<1,60> (e.g. a 60Hz refresh rate) can't be represented exactly with decimal digits, so we want to round from the parsed value to the closest representation in the result type. Truncating is strictly inferior when we're dealing with such periods. The hybrid rounding approach here tries to Do The Right Thing. MSVC seems to avoid this problem by just refusing to parse the date time "2024-09-22 18:34:56" into a result type of sys_days, presumably because the parsed value cannot be represented in the result type. I can see some justification for that, but I think rounding is more useful than failing to parse. Tested x86_64-linux. -- >8 -- I noticed that chrono::parse was using duration_cast and time_point_cast to convert the parsed value to the result. Those functions truncate towards zero, which is not generally what you want. Especially for negative times before the epoch, where truncating towards zero rounds "up" towards the next duration/time_point. Using chrono::round is typically better, as that rounds to nearest. However, while testing the fix I realised that rounding to the nearest can give surprising results in some cases. For example if we parse a chrono::sys_days using chrono::parse("F %T", "2024-09-22 18:34:56", tp) then we will round up to the next day, i.e. sys_days(2024y/09/23). That seems surprising, and I think 2024-09-22 is what most users would expect. This change attempts to provide a hybrid rounding heuristic where we use chrono::round for the general case, but when the result has a period that is one of minutes, hours, days, weeks, or years then we truncate towards negative infinity using chrono::floor. This means that we truncate "2024-09-22 18:34:56" to the start of the current minute/hour/day/week/year, instead of rounding up to 2024-09-23, or to 18:35, or 17:00. For a period of months chrono::round is used, because the months duration is defined as a twelfth of a year, which is not actually the length of any calendar month. We don't want to truncate to a whole number of "months" if that can actually go from e.g. 2023-03-01 to 2023-01-31, because February is shorter than chrono::months(1). libstdc++-v3/ChangeLog: * include/bits/chrono_io.h (__detail::__use_floor): New function. (__detail::__round): New function. (from_stream): Use __detail::__round. * testsuite/std/time/clock/file/io.cc: Check for expected rounding in parse. * testsuite/std/time/clock/gps/io.cc: Likewise. --- libstdc++-v3/include/bits/chrono_io.h | 64 +++++++++++++++++-- .../testsuite/std/time/clock/file/io.cc | 21 +++++- .../testsuite/std/time/clock/gps/io.cc | 20 ++++++ 3 files changed, 97 insertions(+), 8 deletions(-) diff --git a/libstdc++-v3/include/bits/chrono_io.h b/libstdc++-v3/include/bits/chrono_io.h index 1e34c82b532..362bb5aa9e9 100644 --- a/libstdc++-v3/include/bits/chrono_io.h +++ b/libstdc++-v3/include/bits/chrono_io.h @@ -2407,6 +2407,56 @@ namespace __detail template using _Parser_t = _Parser>; + template + consteval bool + __use_floor() + { + if constexpr (_Duration::period::den == 1) + { + switch (_Duration::period::num) + { + case minutes::period::num: + case hours::period::num: + case days::period::num: + case weeks::period::num: + case years::period::num: + return true; + } + } + return false; + } + + // A "do the right thing" rounding function for duration and time_point + // values extracted by from_stream. When treat_as_floating_point is true + // we don't want to do anything, just a straightforward conversion. + // When the destination type has a period of minutes, hours, days, weeks, + // or years, we use chrono::floor to truncate towards negative infinity. + // This ensures that an extracted timestamp such as 2024-09-05 13:00:00 + // will produce 2024-09-05 when rounded to days, rather than rounding up + // to 2024-09-06 (a different day). + // Otherwise, use chrono::round to get the nearest value representable + // in the destination type. + template + constexpr auto + __round(const _Tp& __t) + { + if constexpr (__is_duration_v<_Tp>) + { + if constexpr (treat_as_floating_point_v) + return chrono::duration_cast<_ToDur>(__t); + else if constexpr (__detail::__use_floor<_ToDur>()) + return chrono::floor<_ToDur>(__t); + else + return chrono::round<_ToDur>(__t); + } + else + { + static_assert(__is_time_point_v<_Tp>); + using _Tpt = time_point; + return _Tpt(__detail::__round<_ToDur>(__t.time_since_epoch())); + } + } + } // namespace __detail /// @endcond @@ -2421,7 +2471,7 @@ namespace __detail auto __need = __format::_ChronoParts::_TimeOfDay; __detail::_Parser_t> __p(__need); if (__p(__is, __fmt, __abbrev, __offset)) - __d = chrono::duration_cast>(__p._M_time); + __d = __detail::__round>(__p._M_time); return __is; } @@ -2882,7 +2932,7 @@ namespace __detail else { auto __st = __p._M_sys_days + __p._M_time - *__offset; - __tp = chrono::time_point_cast<_Duration>(__st); + __tp = __detail::__round<_Duration>(__st); } } return __is; @@ -2918,7 +2968,7 @@ namespace __detail // "23:59:60" to correctly produce a time within a leap second. auto __ut = utc_clock::from_sys(__p._M_sys_days) + __p._M_time - *__offset; - __tp = chrono::time_point_cast<_Duration>(__ut); + __tp = __detail::__round<_Duration>(__ut); } return __is; } @@ -2956,7 +3006,7 @@ namespace __detail constexpr sys_days __epoch(-days(4383)); // 1958y/1/1 auto __d = __p._M_sys_days - __epoch + __p._M_time - *__offset; tai_time> __tt(__d); - __tp = chrono::time_point_cast<_Duration>(__tt); + __tp = __detail::__round<_Duration>(__tt); } } return __is; @@ -2995,7 +3045,7 @@ namespace __detail constexpr sys_days __epoch(days(3657)); // 1980y/1/Sunday[1] auto __d = __p._M_sys_days - __epoch + __p._M_time - *__offset; gps_time> __gt(__d); - __tp = chrono::time_point_cast<_Duration>(__gt); + __tp = __detail::__round<_Duration>(__gt); } } return __is; @@ -3020,7 +3070,7 @@ namespace __detail { sys_time<_Duration> __st; if (chrono::from_stream(__is, __fmt, __st, __abbrev, __offset)) - __tp = chrono::time_point_cast<_Duration>(file_clock::from_sys(__st)); + __tp = __detail::__round<_Duration>(file_clock::from_sys(__st)); return __is; } @@ -3049,7 +3099,7 @@ namespace __detail { days __d = __p._M_sys_days.time_since_epoch(); auto __t = local_days(__d) + __p._M_time; // ignore offset - __tp = chrono::time_point_cast<_Duration>(__t); + __tp = __detail::__round<_Duration>(__t); } return __is; } diff --git a/libstdc++-v3/testsuite/std/time/clock/file/io.cc b/libstdc++-v3/testsuite/std/time/clock/file/io.cc index 9da5019ab78..27b4507236e 100644 --- a/libstdc++-v3/testsuite/std/time/clock/file/io.cc +++ b/libstdc++-v3/testsuite/std/time/clock/file/io.cc @@ -67,8 +67,27 @@ test_parse() VERIFY( abbrev == "456" ); VERIFY( offset == (1h + 23min) ); - ss.str(""); + // Test rounding ss.clear(); + ss.str("2224-09-06 23"); + file_time d; + ss >> parse("%F %H", d); // Should be truncated to start of day, not rounded. + ss.str(""); + ss << d; + VERIFY( ss.str() == "2224-09-06 00:00:00" ); + ss.str("1969-12-31 23"); + ss >> parse("%F %H", d); // Should be truncated to start of day, not rounded. + ss.str(""); + ss << d; + VERIFY( ss.str() == "1969-12-31 00:00:00" ); + + file_time>> ds; // decaseconds + ss.str("2224-09-06 15:07:06"); + ss >> parse("%F %T", ds); // Should be rounded to nearest decasecond. + ss << ds; + VERIFY( ss.str() == "2224-09-06 15:07:10" ); + + ss.str(""); ss << file_time{}; VERIFY( ss >> parse("%F %T", tp) ); VERIFY( tp.time_since_epoch() == 0s ); diff --git a/libstdc++-v3/testsuite/std/time/clock/gps/io.cc b/libstdc++-v3/testsuite/std/time/clock/gps/io.cc index c012520080a..647ed021b24 100644 --- a/libstdc++-v3/testsuite/std/time/clock/gps/io.cc +++ b/libstdc++-v3/testsuite/std/time/clock/gps/io.cc @@ -75,6 +75,26 @@ test_parse() VERIFY( abbrev == "GPS" ); VERIFY( offset == -(12h + 34min) ); + // Test rounding + ss.clear(); + ss.str("2224-09-06 23"); + gps_time d; + ss >> parse("%F %H", d); // Should be truncated to start of day, not rounded. + ss.str(""); + ss << d; + VERIFY( ss.str() == "2224-09-06 00:00:00" ); + ss.str("1969-12-31 23"); + ss >> parse("%F %H", d); // Should be truncated to start of day, not rounded. + ss.str(""); + ss << d; + VERIFY( ss.str() == "1969-12-31 00:00:00" ); + + gps_time>> ds; // decaseconds + ss.str("2224-09-06 15:07:06"); + ss >> parse("%F %T", ds); // Should be rounded to nearest decasecond. + ss << ds; + VERIFY( ss.str() == "2224-09-06 15:07:10" ); + ss.str(""); ss << gps_seconds{}; VERIFY( ss >> parse("%F %T", tp) );