[libcxx-commits] [PATCH] D65365: Implement `hh_mm_ss` from P1466R3
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Aug 6 12:06:31 PDT 2019
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)
> 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>
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
More information about the libcxx-commits