[libcxx-commits] [PATCH] D113430: [libc++] Implement file_clock::{to, from}_sys

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/docs/ReleaseNotes.rst:83-84
   ``std::chrono::file_clock::from_time_t``; neither libstdc++ nor MSVC STL
-  had such methods.
+  had such methods. Instead, you can use ``std::chrono::file_clock::from_sys``
+  and ``std::chrono::file_clock::to_sys``, which are specified in the Standard.
 
----------------
Quuxplusone wrote:
> Instead, in C++20 you can use...?
> But what about prior to C++20? Even if the answer is "go jump in a lake," I think it's worth release-noting that. :)
Oh, yeah, somehow I thought the `from_time_t` we removed was C++20 only. Will adjust to basically say "move to C++20", since this whole `file_clock` thing is supposed to be C++20 anyways. For some reason I would have bet that it was C++17 since it is tied to filesystem -- oh well.



================
Comment at: libcxx/test/std/utilities/time/time.clock/time.clock.file/to_from_sys.pass.cpp:59-66
+  // Make sure to_sys and from_sys are consistent with each other
+  {
+    chrono::file_clock::time_point const file_now = chrono::file_clock::now();
+    chrono::system_clock::time_point const sys_now = chrono::system_clock::now();
+    auto sys_diff = chrono::file_clock::to_sys(file_now) - sys_now;
+    auto file_diff = file_now - chrono::file_clock::from_sys(sys_now);
+    assert(sys_diff == file_diff);
----------------
Quuxplusone wrote:
> In many cases we'll have `sys_diff == file_diff == 0`, right?
> I'm not sure I understand the fundamental point of this test; it might be clearer if we had any way of generating constant time_points other than the racey `now()`.
> Do we intend that
> ```
> LIBCPP_ASSERT(std::is_same_v<decltype(sys_diff), decltype(file_diff)>);
> ```
> ?
> Do we intend that `LIBCPP_ASSERT(std::is_same_v<decltype(sys_diff), decltype(file_diff)>);`?

No, we don't. Or at least I don't. In my mind, `decltype(sys_diff)` is `chrono::duration<representation-of-system-clock, period-of-system-clock>` whereas `decltype(file_diff)` is `chrono::duration<representation-of-file-clock, period-of-file-clock>`, and both don't need to be the same. And in fact they aren't in our implementation (system clock has `rep = long long, period = milliseconds` and file clock has `rep = int128_t, period = nanoseconds`).

Both should be "equal", in the sense that if you convert both to the least granular measurement, you should get the same answer. However, both are not necessarily going to equal `0 ticks`, because the two time stamps were not taken at the same time. In practice I expect the difference will be very small because there's almost no time between the two calls to `now()`, but we still should not assume it is 0.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113430/new/

https://reviews.llvm.org/D113430