[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/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:
> ldionne wrote:
> > 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.
> > 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.
> 
> Right; what I meant was more in the line of "flaky tests." Here we have a test where — maybe not //99%// of the time, but //some significant fraction// of the time — it's testing that `0 == 0`; and then some small-and-nonreproducible remainder of the time, it's testing something-other-than-that. This is a "testing code smell."
> 
> It would be strictly better if we could do something like
> ```
>     auto const ft = chrono::file_clock::time_point("2021-01-01 01:23:45");
>     auto const st = chrono::system_clock::time_point("2021-01-01 01:23:45");
>     auto sys_diff = chrono::file_clock::to_sys(ft) - st;
>     auto file_diff = ft - chrono::file_clock::from_sys(st);
>     assert(sys_diff == file_diff);
> ```
> but I don't know that such a primitive for generating timepoints exists (or if it does, it's probably in the `date` stuff we haven't implemented yet).
I agree and would 100% have written the test that way, but as you say, we don't have a good way of generating a time point besides `now()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113430