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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with comments!



================
Comment at: libcxx/CMakeLists.txt:92-93
+# To use time zone support in libc++ the platform needs to have the IANA
+# database installed. Enabling support on platforms without the support in
+# libc++ fail to build. The default is set to the current implementation state
+# on the different platforms.
----------------
Seems clearer.


================
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)); }
+
----------------
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.


================
Comment at: libcxx/include/__chrono/tzdb_list.h:41
+
+// TODO TZDB Document and test _LIBCPP_NODISCARD_EXT.
+class _LIBCPP_AVAILABILITY_TZDB tzdb_list {
----------------
Mordante wrote:
> ldionne wrote:
> > As part of this patch?
> Had a look and I was wondering whether we want to keep documenting our `_LIBCPP_NODISCARD_EXT` since the list is incomplete.
I think we should try to be consistent in this patch. We can talk about not documenting all of the `_LIBCPP_NODISCARD_EXT` in a `rst` document anymore (and I think I'd be in favour of that), but I think we should just be consistent with the current state of the code as of checking this patch in.


================
Comment at: libcxx/test/std/time/time.zone/time.zone.db/time.zone.db.tzdb/tzdb.members.pass.cpp:35
+
+  [[maybe_unused]] std::same_as<std::string> auto _ = tzdb.version = "version";
+
----------------
We also need to do something like `static_assert(std::is_same<decltype(tzdb.version), std::string>)`, don't we?

Right now `tzdb.version` could be anything with an assignment operator that returns a `std::string`, and the test would still pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154282