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

Wed Sep 6 13:25:48 PDT 2023

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

OK, here's a ton of comments. Thanks for addressing them.

================
Comment at: libcxx/include/__chrono/tzdb.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
Why is this its own header?

================
Comment at: libcxx/include/__chrono/tzdb_list.h:30
+
+#  if _LIBCPP_STD_VER >= 20 && !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) &&   \
+      !defined(_LIBCPP_HAS_NO_LOCALIZATION)
----------------
This conjunct deserves its own macro.

================
Comment at: libcxx/include/__chrono/tzdb_list.h:37
+public:
+  _LIBCPP_EXPORTED_FROM_ABI explicit tzdb_list(tzdb&& __tzdb);
+  _LIBCPP_EXPORTED_FROM_ABI ~tzdb_list();
----------------
This should not be a public easily accessible constructor. At minimum it should take an unspellable tag type and not be single argument.

================
Comment at: libcxx/include/__chrono/tzdb_list.h:45
+
+  _LIBCPP_NODISCARD_EXT _LIBCPP_EXPORTED_FROM_ABI const tzdb& front() const noexcept;
+
----------------
And this function can be replaced with `*begin()`. 

The fewer functions we put in the dylib the better. It's not strictly about speed, it's about surface area.  

================
Comment at: libcxx/include/__chrono/tzdb_list.h:49
+
+  _LIBCPP_EXPORTED_FROM_ABI tzdb& __emplace_front(tzdb&& __tzdb);
+
----------------
It seems unnecessary to export this function given that it's only used internally.

Please remove it.

================
Comment at: libcxx/include/__chrono/tzdb_list.h:54
+
+  _LIBCPP_NODISCARD_EXT _LIBCPP_EXPORTED_FROM_ABI const_iterator cbegin() const noexcept;
+  _LIBCPP_NODISCARD_EXT _LIBCPP_EXPORTED_FROM_ABI const_iterator cend() const noexcept;
----------------
Please remove these two functions from the dylib and instead just have them call `begin()` and `end()`. 

================
Comment at: libcxx/include/__config:401
 #    define _LIBCPP_HAS_NO_EXPERIMENTAL_STOP_TOKEN
+#    define _LIBCPP_HAS_NO_INCOMPLETE_TZDB
 #  endif
----------------
This macro is poorly named.

================
Comment at: libcxx/modules/std/chrono.inc:213
+
+#if 0
     // [time.zone.exception], exception classes
----------------
I understand this `#if 0` was existing.

But it's never OK practice to check into source `#if 0`. In particular without any explanation as to why it exists.

Code that needs to be wrapped in `#if 0` isn't ready to be checked in.

================
Comment at: libcxx/src/CMakeLists.txt:329

+if (LIBCXX_ENABLE_LOCALIZATION AND LIBCXX_ENABLE_FILESYSTEM AND LIBCXX_ENABLE_TIME_ZONE_DATABASE)
+  list(APPEND LIBCXX_EXPERIMENTAL_SOURCES
----------------
Why are you checking three options here?

It seems like if LIBCXX_ENABLE_TIME_ZONE_DATABASE is set, we should either error because we can't provide it without localization and filesystem, or we should provide it.

================
Comment at: libcxx/src/tz.cpp:77
+
+[[nodiscard]] static string __parse_string(istream& __input) {
+  string __result;
----------------
Nit. It doesn't look like this function "parses a string".

================
Comment at: libcxx/src/tz.cpp:107
+  chrono::__skip_optional_whitespace(__input);
+  chrono::__matches(__input, "version");
+  chrono::__skip_mandatory_whitespace(__input);
----------------
`__matches` sounds like it's a predicate. `__consume_match` makes more sense.

Also you don't need ugly identifiers inside source files. And its preferable that you don't use them.
It makes it appear as though these function are exported from the dylib or are from the headers.

================
Comment at: libcxx/src/tz.cpp:127
+_LIBCPP_NODISCARD_EXT _LIBCPP_AVAILABILITY_TZDB _LIBCPP_EXPORTED_FROM_ABI tzdb_list& get_tzdb_list() {
+  static tzdb_list __result{chrono::__make_tzdb()};
+  return __result;
----------------
Should this be `nodestroy`?

================
Comment at: libcxx/src/tz.cpp:127
+_LIBCPP_NODISCARD_EXT _LIBCPP_AVAILABILITY_TZDB _LIBCPP_EXPORTED_FROM_ABI tzdb_list& get_tzdb_list() {
+  static tzdb_list __result{chrono::__make_tzdb()};
+  return __result;
----------------
EricWF wrote:
> Should this be `nodestroy`?
I would find a different way to initialize this that doesn't require having a public constructor.

================
Comment at: libcxx/src/tz.cpp:140
+  filesystem::path __root = chrono::__libcpp_tzdb_directory();
+  ifstream __tzdata{__root / "tzdata.zi"};
+  return chrono::__parse_version(__tzdata);
----------------
This whole parsing bit seems overly complicated considering the pattern it's matching.
Can't we just read some large chunk out of the file, and then use `string_view` to walk it and parse it?

================
Comment at: libcxx/src/tzdb_list.cpp:42
+
+  const_iterator erase_after(const_iterator __p) {
+#ifndef _LIBCPP_HAS_NO_THREADS
----------------
Doesn't this operation make the whole "thread safety" bit moot?

If you can erase an arbitrary member in the list, then that erase is potentially racy for any iterator on or before that element?

So either all the locking is unneeded, or we need to replace forward_list with a hand rolled atomic linked list. (Which shouldn't be too hard given the limited operations it needs to support).

================
Comment at: libcxx/src/tzdb_list.cpp:72
+#ifndef _LIBCPP_HAS_NO_THREADS
+  mutable shared_mutex __mutex_;
+#endif
----------------
Why is this a shared_mutex? 

================
Comment at: libcxx/src/tzdb_list.cpp:83
+
+_LIBCPP_EXPORTED_FROM_ABI tzdb_list::~tzdb_list() { delete __impl_; }
+
----------------
This should never be called. 

================
Comment at: libcxx/test/libcxx/diagnostics/chrono.nodiscard_extensions.compile.pass.cpp:25
+#include "test_macros.h"
+
+void test() {
----------------
Just make these normal tests and call the functions.

================
Comment at: libcxx/test/libcxx/experimental/fexperimental-library.compile.pass.cpp:34
+
+#ifdef _LIBCPP_HAS_NO_INCOMPLETE_TZDB
+#  error "-fexperimental-library should enable the chrono TZDB"
----------------
So it has complete TZDB?

================
Comment at: libcxx/test/std/time/time.zone/time.zone.db/time.zone.db.list/iterators.pass.cpp:12
+
+// XFAIL: libcpp-has-no-incomplete-tzdb
+// XFAIL: availability-tzdb-missing
----------------
This is expected to fail if we have a complete tzdb implementation?

================
Comment at: libcxx/test/support/test_tzdb.h:23
+// suite. Therefore the declaration is not in <chrono>.
+_LIBCPP_AVAILABILITY_TZDB _LIBCPP_OVERRIDABLE_FUNC_VIS string_view __libcpp_tzdb_directory();
+
----------------
This is a clever way to test the internals. Good thinking.

Though it feels like this should probably return a string?

================
Comment at: libcxx/utils/ci/buildkite-pipeline.yml:684

+    - label: "No time zone database"
+      command: "libcxx/utils/ci/run-buildbot generic-no-tzdb"
----------------
Please remove this configuration.

We don't have the capacity for it.

================
Comment at: libcxx/utils/ci/run-buildbot:462
 ;;
+generic-no-tzdb)
+    clean
----------------
this doesn't need its own configuration right now.

It's also not clear to me that we need a configuration switch here.

If the user doesn't touch the tzdb interface, the fact that there is no database won't crop up.
So introducing a whole new configuration here is a bad idea.

Also, we effectively test this configuration when we test without filesystem.
Please remove it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154282