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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/docs/Status/Cxx20.rst:54
    .. [#note-P0660] P0660: Section 32.3 Stop Tokens is complete. ``jthread`` hasn't been implemented yet.
+   .. [#note-P0355] P0355: The implementation status is:
+
----------------
Maybe a status page would be appropriate?


================
Comment at: libcxx/docs/UsingLibcxx.rst:577-580
+   std::string_view std::chrono::__libcpp_tzdb_directory() {
+     static std::string_view result = "C:/usr/share/zoneinfo";
+     return result;
+   }
----------------
Why do you make this a static? The string literal already has static storage duration.


================
Comment at: libcxx/include/__chrono/tzdb.h:19
+
+#  include <string>
+
----------------
Including headers shouldn't depend on `-fexperimental-library`. Same for `#pragma GCC system_header`.


================
Comment at: libcxx/include/__config:297
+#  if !defined(_LIBCPP_ENABLE_EXPERIMENTAL) && !defined(_LIBCPP_BUILDING_LIBRARY)
+#    define _LIBCPP_HAS_NO_INCOMPLETE_TZDB
+#  endif
----------------
Why isn't the macro simply defined in the `#ifdef` above?


================
Comment at: libcxx/src/tz.cpp:42
+
+[[nodiscard]] static bool __is_whitespace(int __c) { return __c == ' ' || __c == '\t'; }
+
----------------
Why don't you put this in an anonymous namespace instead? Also, these functions and arguments don't have to be _Uglified, since they are only in the sources. I would also move this into the global namespace to avoid confusion.


================
Comment at: libcxx/src/tz.cpp:124-140
+_LIBCPP_NODISCARD_EXT _LIBCPP_AVAILABILITY_TZDB _LIBCPP_EXPORTED_FROM_ABI tzdb_list& get_tzdb_list() {
+  static tzdb_list __result{chrono::__make_tzdb()};
+  return __result;
+}
+
+_LIBCPP_AVAILABILITY_TZDB _LIBCPP_EXPORTED_FROM_ABI const tzdb& reload_tzdb() {
+  if (chrono::remote_version() == chrono::get_tzdb().version)
----------------
I would keep all the annotations exclusively in the headers. That makes this code much nicer to read.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154282