[libcxx-commits] [PATCH] D154282: [libc++][chrono] Adds tzdb_list implementation.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 5 inline comments as done.
Mordante added a comment.

Thanks for the review!



================
Comment at: libcxx/include/__chrono/tzdb_list.h:44
+public:
+  _LIBCPP_HIDE_FROM_ABI explicit tzdb_list(tzdb&& __tzdb) { __tzdb_.push_front(std::move(__tzdb)); }
+
----------------
ldionne wrote:
> Have you considered making this a pimpl to hide the definition of the class into the dylib? It might help with keeping the added dependencies minimal, and it would also give us more freedom to change the implementation in the future (although IDK whether we'll ever need/want that freedom). I'm not asking you to do that, but I think it should at least be considered.
I had not really considered that since it's typically not something we do. The big disadvantage of the pimpl pattern is that it moves code to the dylib causing it not to be available on Apple platforms. In this case that is not a real issue; other parts of the TZDB are already in the dylib.

Calling the dylib might give some overhead. I don't think that is a huge issue. Looking at the amount of work needed to use time zones I don't see it as very fast code in the first place.


================
Comment at: libcxx/include/module.modulemap.in:786
+      module tzdb {
+        private header "__chrono/tzdb.h"
+        export string
----------------
Mordante wrote:
> Add  `@requires_LIBCXX_ENABLE_LOCALIZATION@` and filesystem here and next file.
No longer needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154282