[PATCH] D25416: Add Chrono.h - std::chrono support header

[Zachary Turner via llvm-commits]

zturner added inline comments.


================
Comment at: include/llvm/Support/Chrono.h:26-27
+/// implementation-defined).
+using TimePoint = std::chrono::time_point<std::chrono::system_clock,
+                                          std::chrono::nanoseconds>;
+
----------------
I'm not sure if it's appropriate to have a generic name like `TimePoint` (which indicates it should be used for any and all time calculations) be forced to a specific resolution.  A couple of possibilities would be:

1. Call it `TimePointNano`
2. making it an alias template (e.g. `template<typename R> using TimePoint = time_point<system_clock, T>`)


================
Comment at: include/llvm/Support/Chrono.h:30
+/// Convert a TimePoint to std::time_t
+inline std::time_t toTimeT(TimePoint TP) {
+  using namespace std::chrono;
----------------
Any reason this function shouldn't be templated to work with arbitrary resolutions?

Also, I would use `LLVM_ATTRIBUTE_ALWAYS_INLINE` instead of the `inline` keyword.


================
Comment at: include/llvm/Support/Chrono.h:37
+/// Convert a std::time_t to a TimePoint
+inline TimePoint toTimePoint(std::time_t T) {
+  return std::chrono::system_clock::from_time_t(T);
----------------
Same.


================
Comment at: include/llvm/Support/TimeValue.h:117
+    TimeValue(TimePoint TP)
+        : seconds_(sys::toTimeT(TP) + PosixZeroTimeSeconds),
+          nanos_((TP.time_since_epoch() % std::chrono::seconds(1)).count()) {}
----------------
I might be missing something, but `PosixZeroTimeSeconds` is defined in a cpp file but you're using it in a header file.  And it's not forward declared anywhere?


================
Comment at: lib/Support/Windows/Chrono.inc:23
+
+raw_ostream &operator<<(raw_ostream &OS, TimePoint TP) {
+  struct tm *LT;
----------------
The windows and non-windows versions appear identical except for the call to `_localtime64_s` vs `localtime_r`.  This is a case where the implementations are similar enough that I think it does more harm than good to separate the files like this.  I would put one function in `Chrono.cpp` with a preprocessor directive to select either `_localtime64_s` or `localtime_r`.


https://reviews.llvm.org/D25416