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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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.
 
----------------
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. :)


================
Comment at: libcxx/test/std/utilities/time/time.clock/time.clock.file/to_from_sys.pass.cpp:33-36
+    chrono::file_clock::time_point const file_now = chrono::file_clock::now();
+    auto sys_now = chrono::file_clock::to_sys(file_now);
+    assert(file_now == chrono::file_clock::from_sys(sys_now));
+  }
----------------
Naming nit: When I see various variables named `foo_now`, it fires my red-flag detector for TOCTTOU vulnerabilities. In this case, `file_now` is arguably a "now," but `sys_now` is clearly a "then," because by the time we've computed it, "now" is later than before. ;)  I suggest just `ft` and `st`, here and below. (You can still initialize `ft` with `file_clock::now()` if you're confident you won't mind whatever's going to happen to this test on DST day or whatever. I just don't like the variable's own name to include the word `now`.)


================
Comment at: libcxx/test/std/utilities/time/time.clock/time.clock.file/to_from_sys.pass.cpp:46-57
+  {
+    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 diff = sys_now - chrono::file_clock::to_sys(file_now);
+      assert(diff < chrono::milliseconds{500});
+    }
----------------
Here, this is a //good// use of variables named `foo_now`. :)
I'd recommend testing that
```
assert(std::chrono::milliseconds(-500) < diff && diff < std::chrono::milliseconds(500));
```
just to be on the safe side. (And notice my usual preference for `()` in object-factory-style constructors, idiomatically reserving `{}` for braced initializer lists.)


================
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);
----------------
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)>);
```
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113430