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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/CMakeLists.txt:94
+  ship the IANA time zone database. When time zones are not supported,
+  time zone support in <chrono> will be disabled." ON)
 option(LIBCXX_ENABLE_VENDOR_AVAILABILITY_ANNOTATIONS
----------------
We could instead disable this by default except on Linux. This would avoid breaking the build for everyone on major platforms right now (which would happen if we switch to `#error` in the dylib).


================
Comment at: libcxx/docs/Status/Cxx20.rst:54
+
+      * ``Calendars`` mostely done in Clang 7
+      * ``Input parsers`` not done
----------------



================
Comment at: libcxx/docs/UsingLibcxx.rst:528
 
+Extensions to ``<chrono>``
+--------------------------
----------------
I think we should document this in a separate page with implementation-defined behavior.


================
Comment at: libcxx/include/__availability:179
+    // The parser code is built in the library.
+// #   define _LIBCPP_AVAILABILITY_TZDB
+#   define _LIBCPP_AVAILABILITY_TZDB
----------------



================
Comment at: libcxx/include/__availability:356
 
+#  define _LIBCPP_AVAILABILITY_TZDB __attribute__((unavailable))
+
----------------



================
Comment at: libcxx/include/__chrono/tzdb_list.h:41
+
+// TODO TZDB Document and test _LIBCPP_NODISCARD_EXT.
+class _LIBCPP_AVAILABILITY_TZDB tzdb_list {
----------------
As part of this patch?


================
Comment at: libcxx/include/__chrono/tzdb_list.h:79
+  _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI const_iterator end() const noexcept {
+    //  forward_list<T>::end does not access the list.
+    return __tzdb_.end();
----------------



================
Comment at: libcxx/include/__chrono/tzdb_list.h:93-96
+// This function allows users to override the directory where the TZDB
+// files are stored. This should rarely be needed, a better solution
+// would be to set the proper value at build time in
+// _LIBCPP_CHRONO_TZDB_DIRECTORY
----------------
I think we should try to make it as clear as possible that this is not intended to be used by users.

Actually, we don't have to declare it here at all, we can only declare + override it in the test suite. We should add a comment explaining that this is necessary for testing in `tz.cpp` where that function is marked as `_LIBCPP_WEAK`.


================
Comment at: libcxx/include/__config:1478-1485
+// Libc++ support for the IANA Time Zone Database.
+// TODO TZDB Add Apple when it provides the required files.
+// TODO TZDB Add FreeBSD when it provides the required files.
+#  ifndef _LIBCPP_HAS_NO_TIME_ZONE_DATABASE
+#    if defined(__linux__)
+#      define _LIBCPP_TIME_ZONE_DB_PATH "/usr/share/zoneinfo/"
+#    endif
----------------
Since we need to implement `__libcpp_tzdb_directory` in the dylib anyway, I think we should move this to `tz.cpp` to keep it as localized as possible.


================
Comment at: libcxx/include/chrono:835
 
+#if !defined(_LIBCPP_HAS_NO_FILESYSTEM) && !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+#  include <__chrono/tzdb.h>
----------------
Is this missing `_LIBCPP_HAS_NO_TIME_ZONE_DATABASE`?


================
Comment at: libcxx/src/tz.cpp:22
+// https://data.iana.org/time-zones/tz-how-to.html and
+// https://man7.org/linux/man-pages/man8/zic.8.html .
+//
----------------



================
Comment at: libcxx/src/tz.cpp:118
+#ifndef _LIBCPP_TIME_ZONE_DB_PATH
+  std::__throw_runtime_error("unknown path to the IANA Time Zone Database");
+#else
----------------
This should be a `#error` instead.


================
Comment at: libcxx/test/libcxx/transitive_includes.gen.py:67
 // TODO: Fix this test to make it work with localization or wide characters disabled
-// UNSUPPORTED{BLOCKLIT}: no-localization, no-wide-characters
+// UNSUPPORTED{BLOCKLIT}: no-localization, no-wide-characters, no-threads, no-filesystem, libcpp-has-no-incomplete-tzdb
 
----------------
Also `no-tzdb`?


================
Comment at: libcxx/utils/libcxx/test/features.py:570
+    Feature(
+        name="no-system-provided-tzdb",
+        when=lambda cfg: BooleanExpression.evaluate(
----------------
This one would be removed if we turned tzdb support to OFF by default on platforms that don't support it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154282