[libcxx-commits] [PATCH] D65365: Implement `hh_mm_ss` from P1466R3

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D65365#1616172 <https://reviews.llvm.org/D65365#1616172>, @mclow.lists wrote:

> We have a "power of 10" table in `charconv`, but I don't think that coupling `<chrono>` and `<charconv>` is worth the benefit of removing this small routine.


We've discussed that offline, however I want to say it here for posterity. The fact that we have monolithic headers in libc++ sometimes leads us to duplicate code (like here). If instead we had finer grained headers, we could reuse code without fear of adding coupling between unrelated headers.

I was told there was a fear that adding header files would increase compile-times, however I haven't personally seen that claim substantiated. Also, having finer grained headers means that we might be able to pull in less code when we include something.



================
Comment at: libcxx/include/chrono:2757
+
+    static constexpr uint64_t __pow10(unsigned __exp)
+    {
----------------
mclow.lists wrote:
> ldionne wrote:
> > It seems like this shouldn't be reimplemented inside this class, instead we should put this at namespace scope if we don't already have something like that.
> I put it in the class so it wouldn't be polluting the enclosing namespace.  If it's generally useful, we can promote it.
I'd like it to be promoted so it can be tested on its own.


================
Comment at: libcxx/include/chrono:617
+template <class Duration>
+class hh_mm_ss
+{
----------------
When do you plan do update the feature-test macro `__cpp_lib_chrono`?


================
Comment at: libcxx/test/std/utilities/time/time.hms/time.hms.members/hours.pass.cpp:16
+   
+// Test values
+// duration     hours   minutes seconds fractional              count
----------------
Can you add a comment in the other related test files saying there's a correspondence table in this file? Something like:

```
// See the table in hours.pass.cpp for correspondence between the magic values used below
```


================
Comment at: libcxx/test/std/utilities/time/time.hms/time.hms.nonmembers/nothing.to.do.pass.cpp:11
+
+// template <class Duration> class hh_mm_ss;
+
----------------
What is this testing?


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

https://reviews.llvm.org/D65365