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

[Pavel Labath via llvm-commits]

labath added a comment.

Responses to comments inline. I will upload a new version of the code tomorrow.



================
Comment at: include/llvm/Support/Chrono.h:26-27
+/// implementation-defined).
+using TimePoint = std::chrono::time_point<std::chrono::system_clock,
+                                          std::chrono::nanoseconds>;
+
----------------
zturner wrote:
> 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>`)
I wanted to make the usage as frictionless as possible (and TimeValue already used nanoseconds precision and noone seemed to be bothered by that), but I do see your point. How about we go for the second option, but we have a default template parameter, so people don't have to specify precision unless they actually need to. I have two counter-proposals:
- `template<typename D = nanoseconds> using TimePoint = time_point<system_clock, D>`, with the usage being: `TimePoint<std::chrono::microseconds>`
- `template<typename R = nano> using TimePoint = time_point<system_clock, duration<XXX, R>>`, with the usage being `TimePoint<std::micro>`

I'd tend towards the second option as it is shorter if you don't import std::chrono namespace, but I don't care too much, as I don't anticipate much usage with a custom precision.


================
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;
----------------
zturner wrote:
> 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.
All (reasonable) time_points are implicitly convertible to a time_point with nanosecond precision, so this will already work with all of them, without the need for templating. The only difference I see is that a time_point with second precision will probably have higher range than the nanosecond one, but I don't think we ought to be relying on anywhere anyway.


================
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);
----------------
zturner wrote:
> Same.
I suppose we could return a time_point with second precision, as that is all that's stored in a time_t anyway. I don't think we need templates, because of implicit conversions.


================
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()) {}
----------------
zturner wrote:
> 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?
It's declared in this file, line 375.


================
Comment at: lib/Support/Windows/Chrono.inc:23
+
+raw_ostream &operator<<(raw_ostream &OS, TimePoint TP) {
+  struct tm *LT;
----------------
zturner wrote:
> 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`.
Sounds like a good idea.


https://reviews.llvm.org/D25416