-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[libcxx] Implement C++20 std::chrono::is_clock, std::chrono::is_clock_v #160607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-libcxx Author: Yuxuan Chen (yuxuanchen1997) ChangesImplemented [time.traits.is.clock] from P0355. Full diff: https://github.com/llvm/llvm-project/pull/160607.diff 5 Files Affected:
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index e050362abb658..210ea8be4a69d 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -261,6 +261,7 @@ set(files
__chrono/formatter.h
__chrono/gps_clock.h
__chrono/hh_mm_ss.h
+ __chrono/is_clock.h
__chrono/high_resolution_clock.h
__chrono/leap_second.h
__chrono/literals.h
diff --git a/libcxx/include/__chrono/is_clock.h b/libcxx/include/__chrono/is_clock.h
new file mode 100644
index 0000000000000..dc64c9eacd80b
--- /dev/null
+++ b/libcxx/include/__chrono/is_clock.h
@@ -0,0 +1,45 @@
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___CHRONO_IS_CLOCK_H
+#define _LIBCPP___CHRONO_IS_CLOCK_H
+
+#include <__config>
+
+#if _LIBCPP_STD_VER >= 20
+
+# include <__type_traits/integral_constant.h>
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+namespace chrono {
+
+template <class>
+struct is_clock : std::false_type {};
+
+template <class _Tp>
+ requires requires {
+ typename _Tp::rep;
+ typename _Tp::period;
+ typename _Tp::duration;
+ typename _Tp::time_point;
+ _Tp::is_steady;
+ _Tp::now();
+ }
+struct is_clock<_Tp> : std::true_type {};
+
+template <class _Tp>
+_LIBCPP_NO_SPECIALIZATIONS inline constexpr bool is_clock_v = is_clock<_Tp>::value;
+
+} // namespace chrono
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP_STD_VER
+#endif // _LIBCPP___CHRONO_IS_CLOCK_H
diff --git a/libcxx/include/chrono b/libcxx/include/chrono
index 82e99a31bcc9f..f3bb08ef386c9 100644
--- a/libcxx/include/chrono
+++ b/libcxx/include/chrono
@@ -1057,6 +1057,7 @@ constexpr chrono::year operator ""y(unsigned lo
# include <__chrono/day.h>
# include <__chrono/exception.h>
# include <__chrono/hh_mm_ss.h>
+# include <__chrono/is_clock.h>
# include <__chrono/literals.h>
# include <__chrono/local_info.h>
# include <__chrono/month.h>
diff --git a/libcxx/modules/std/chrono.inc b/libcxx/modules/std/chrono.inc
index 66eccd8d290ad..db405d482bf9e 100644
--- a/libcxx/modules/std/chrono.inc
+++ b/libcxx/modules/std/chrono.inc
@@ -25,8 +25,8 @@ export namespace std {
using std::chrono::duration_values;
- // using std::chrono::is_clock;
- // using std::chrono::is_clock_v;
+ using std::chrono::is_clock;
+ using std::chrono::is_clock_v;
// [time.duration.nonmember], duration arithmetic
using std::chrono::operator+;
diff --git a/libcxx/test/std/time/time.traits.is.clock/trait.is.clock.pass.cpp b/libcxx/test/std/time/time.traits.is.clock/trait.is.clock.pass.cpp
new file mode 100644
index 0000000000000..b15e91c48b0be
--- /dev/null
+++ b/libcxx/test/std/time/time.traits.is.clock/trait.is.clock.pass.cpp
@@ -0,0 +1,22 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: std-at-least-c++20
+
+#include <chrono>
+
+struct NotAClock {};
+
+int main(int, char**) {
+ static_assert(!std::chrono::is_clock_v<NotAClock>, "should not be treated as a clock");
+
+ static_assert(std::chrono::is_clock_v<std::chrono::system_clock>);
+ static_assert(std::chrono::is_clock_v<std::chrono::steady_clock>);
+ static_assert(std::chrono::is_clock_v<std::chrono::high_resolution_clock>);
+ return 0;
+}
|
d25ec00 to
ec64186
Compare
ec64186 to
977db81
Compare
977db81 to
74f494a
Compare
libcxx/test/std/time/time.traits.is.clock/trait.is.clock.compile.pass.cpp
Show resolved
Hide resolved
libcxx/test/std/time/time.traits.is.clock/trait.is.clock.compile.pass.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: A. Jiang <[email protected]>
Would it make sense to create a sub-task to track this item in #99982? |
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions ,inc,h,cpp -- libcxx/include/__chrono/is_clock.h libcxx/test/std/time/time.traits.is.clock/trait.is.clock.compile.pass.cpp libcxx/test/std/time/time.traits.is.clock/trait.is.clock.compile.verify.cpp libcxx/include/chrono libcxx/modules/std/chrono.inc --diff_from_common_commit
View the diff from clang-format here.diff --git a/libcxx/include/__chrono/is_clock.h b/libcxx/include/__chrono/is_clock.h
index 543181bf2..c064d14f9 100644
--- a/libcxx/include/__chrono/is_clock.h
+++ b/libcxx/include/__chrono/is_clock.h
@@ -37,8 +37,8 @@ template <class _TimePoint, class _ClockType>
inline constexpr bool __is_valid_clock_time_point_v = false;
template <class _TimePointClock, class _ClockType>
-inline constexpr bool __is_valid_clock_time_point_v<time_point<_TimePointClock, typename _ClockType::duration>, _ClockType> =
- true;
+inline constexpr bool
+ __is_valid_clock_time_point_v<time_point<_TimePointClock, typename _ClockType::duration>, _ClockType> = true;
// Check if a clock satisfies the Cpp17Clock requirements as defined in [time.clock.req]
template <class _Tp>
|
|
After some feedback, we decided to implement more checks based on the
The set of requirements for |
8a6adc0 to
bd7ca12
Compare
libcxx/test/std/time/time.traits.is.clock/trait.is.clock.compile.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/time/time.traits.is.clock/trait.is.clock.compile.pass.cpp
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. CC @philnik777
@H-G-Hristov I've just created the corresponding subtask. @yuxuanchen1997 I modified the PR description to associate this PR with the corresponding subtask. Please double-check. |
| template <class _Tp> | ||
| _LIBCPP_NO_SPECIALIZATIONS inline constexpr bool is_clock_v = requires { | ||
| typename _Tp::rep; | ||
| requires is_arithmetic_v<typename _Tp::rep>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong. Cpp17Clock requires the type to be "an arithmetic type or a class emulating an arithmetic type". I don't think "a class emulating an arithmetic type" is defined anywhere, but IMO we should rather be permissive here than strict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify what a permissive outcome might be? Do we want to drop this requires? Or do we want a different check based on https://eel.is/c++draft/time.clock.req#:~:text=a%20class%20emulating%20an%20arithmetic%20type ?
| template <class _Rep, class _Period> | ||
| class duration; | ||
|
|
||
| template <class _Clock, class _Duration> | ||
| class time_point; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't forward declare classes in random places. Either include the appropriate header or make a forward declaring header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should introduce a forward declaring header here.
|
|
||
| // Test standard clock types | ||
| static_assert(std::chrono::is_clock_v<std::chrono::system_clock>); | ||
| #if _LIBCPP_HAS_MONOTONIC_CLOCK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't use libc++-internal macros to guard against this stuff. More generally, do we really need to test this? I don't think this adds much value while making things definitely more complicated.
| @@ -0,0 +1,28 @@ | |||
| //===----------------------------------------------------------------------===// | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a standard test, so it should live inside test/libcxx.
| #if !__has_warning("-Winvalid-specializations") | ||
| // expected-no-diagnostics | ||
| #else | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still required? The minimum supported Clang version is 20 now, where the attribute has been implemented already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I added it for some CI failures. I can try removing.
| // expected-no-diagnostics | ||
| #else | ||
|
|
||
| namespace std::chrono { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid opening namespace std.
Co-authored-by: Nikolas Klauser <[email protected]>
Co-authored-by: A. Jiang <[email protected]>
Co-authored-by: Nikolas Klauser <[email protected]>
Implemented [time.traits.is.clock] from P0355R7.
This patch implements the C++20 feature
is_clockandis_clock_vbased on the documentation on cppreferenceFixes #166049.