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

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: unittests/Support/Chrono.cpp:36
+  raw_string_ostream OS(S);
+  OS << system_clock::now();
+
----------------
labath wrote:
> 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.
I missed that the conversion was implicit here.


https://reviews.llvm.org/D25416