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

[Pavel Labath via llvm-commits]

labath added inline comments.


================
Comment at: unittests/Support/Chrono.cpp:36
+  raw_string_ostream OS(S);
+  OS << system_clock::now();
+
----------------
mehdi_amini wrote:
> I initially thought this test (StringConversion ) would be doing something else (`TimePoint` string conversion), but both are valuable so what do you think about another test like:
> 
> ```
>   auto Now = system_clock::now();
>   
>   std::string S1;
>   {
>     raw_string_ostream OS(S1);
>     OS << system_clock::now();
>    }
> 
>   TimePoint T(Now);
>   std::string S2;
>   {
>     raw_string_ostream OS(S2);
>     OS << T;
>    }
>    ASSERT_EQ(S1, S2);
> }
> ```
> 
> (I don't think you added coverage for `raw_ostream &operator<<(raw_ostream &OS, TimePoint TP) {`, it is likely covered by the `TimeValue` tests (if any), but if we remove TimeValue at some point we may lose the coverage.
TimePoint is just a typedef for a pre-existing STL type that is implicitly constructible from the result of `system_clock::now()`. So, `OS << system_clock::now()` will actually invoke the  `raw_ostream &operator<<(raw_ostream &OS, TimePoint TP)`. The only difference between `OS << system_clock::now()`, and `OS << TimePoint(system_clock::now()` is that in the latter, the conversion operation is explicit (i.e., the newly introduced operator<< will have coverage even when TimeValue is removed, which I plan to do soon).

I can add the test you mentioned, but I don't think it is that useful, as it will just compare two invocations of the same function. I can also add an explicit TimePoint cast to the existing test, if you think that will make things clearer.


https://reviews.llvm.org/D25416