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

Tue Aug 15 09:39:47 PDT 2023

ldionne added inline comments.

================
Comment at: libcxx/CMakeLists.txt:307

+set(LIBCXX_TIME_ZONE_DB "" CACHE PATH
+  "Where the time zone data files are found. When the path is empty
----------------
I think we should remove the ability to set a custom path and always use the system defaults. We should work with the various platforms we support to make sure that they provide these resources, and if they don't then it would be reasonable for that part of functionality not to be supported at all.

================
Comment at: libcxx/CMakeLists.txt:314
+endif()
+
 #===============================================================================
----------------
However, we should add something like `LIBCXX_ENABLE_TIME_ZONE_SUPPORT`, which would act like `LIBCXX_ENABLE_FILESYSTEM` and others. I would expect e.g. embedded systems to turn that off, but it should be on by default.

================
Comment at: libcxx/docs/BuildingLibcxx.rst:25

+.. _TimeZoneDatabase:
+
----------------
I don't think this documentation is required anymore if we assume platform support when `LIBCXX_ENABLE_TIME_ZONE_SUPPORT=ON`.

================
Comment at: libcxx/docs/BuildingLibcxx.rst:328

+.. option:: LIBCXX_TIME_ZONE_DB:PATH
+
----------------
This goes too.

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:19
+
+The data of the database is available in several platforms in different forms:
+
----------------

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:45-48
+The disadvantage of the binary data is that it's not possible on all platforms
+to get the proper ``time_zone_link`` information. This information is in the
+database, this difference is observable by users. Without the database version
+information it's not possible to create a conforming implementation.
----------------

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:50-53
+Since it's easier to parse one file than a set of files the libc++ team decided
+to use the ``tzdata.zi``. The other benefit is that the ``tzdata.zi`` file
+contains the database version information needed for a conforming
+implementation.
----------------

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:55-60
+The file ``tzdata.zi`` is not available on all platforms, so vendors need to
+make changes for their platform.  Most vendors already ship the database, so
+they need to adjust the packaging of their time zone package. On Windows this
+is not available by default. However it's possible for Windows packagers to add
+these files to their libc++ packages. The IANA databases can be
+`downloaded <https://data.iana.org/time-zones/releases/>`_.
----------------

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:64-65
+compiled in the dylib. The text file is about 112 KB. For now libc++ will not
+ship this file. When it's hard to get vendors to ship these files we can
+reconsider based on that information.
+
----------------

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:80
+
+There are several mechanisms to select the database location
+
----------------

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:85
+  ``include/__config``.
+- The CMake configuration option ``LIBCXX_TIME_ZONE_DB``. This makes it
+  possible for vendors to ship the database when its location is unknown to
----------------
This will go away.

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:91
+  system wide.
+- Overriding ``std::chrono::__libcpp_tzdb_directory()``. This allows
+  applications to specify their own database path and implement their own time
----------------
Point of discussion: do we really want to let users override this? Unless we have clear uses cases for that, I would recommend not exposing this to users at first. We can do something ad-hoc for our testing needs.

If we decide not to expose this to users, let's make it clear by not saying `"this allows applications to specify their own database path [...]"`.

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:114
+There is an update mechanism in libc++, however this is not done automatically.
+Invoking the function ``std::chrono:: remote_version()`` will parse the version
+information of the ``tzdata.zi`` file and returns that information. Similar
----------------

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:115-118
+information of the ``tzdata.zi`` file and returns that information. Similar
+``std::chrono::reload_tzdb()`` will parse the ``tzdata.zi`` and
+``leap-seconds.list`` again. This makes is possible to update the database if
+needed by the application and gives the user full power over the update policy.
----------------

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:127-130
+- When there is no threading available this polling
+  becomes more involved. For example, query the file every *x* calls to
+  ``std::chrono::get_tzdb()``. This mean calls to ``std::chrono::get_tzdb()``
+  has different performance characteristics.
----------------

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:131-135
+- Even when the automatic update is implemented, user may want
+  ``std::chrono:: remote_version()`` to load the most recent information. For
+  example since they use ``std::chrono::__libcpp_tzdb_directory()`` to have
+  full control over the time zone information in their application.
+  That means ``std::chrono:: remote_version()`` should always query.
----------------

================
Comment at: libcxx/docs/DesignDocs/TimeZone.rst:143-145
+Another issue with the automatic update is, that is may not be considered
+Standard compliant "This update shall not impact the program in any way". Using
+resources could be considered impacting the program.
----------------

================
Comment at: libcxx/docs/UsingLibcxx.rst:531
+
+Configuring the Time Zone Database location
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
----------------
This should be updated.

================
Comment at: libcxx/docs/UsingLibcxx.rst:589
+
+- ``std::chrono::remote_version()`` will update the verson information of the
+  *remote time zone database*,
----------------

================
Comment at: libcxx/docs/UsingLibcxx.rst:594-595
+
+This offers a method users to update the *remote time zone database*, and
+giving them full control over the process.
+
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154282