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

Thu Sep 7 12:07:24 PDT 2023

Mordante marked 5 inline comments as done.
Mordante added a comment.

@EricWF Please ping me on Discord when you have finished the review, then I will look at a followup patch.

================
Comment at: libcxx/include/__chrono/tzdb.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
EricWF wrote:
> Why is this its own header?
Because nowadays (for at least two years) we tend to work with small headers. I'm open to discuss that, but then we should have a general policy discussion about this and not in individual patches.

================
Comment at: libcxx/modules/std/chrono.inc:213
+
+#if 0
     // [time.zone.exception], exception classes
----------------
EricWF wrote:
> 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.
In general I agree with that statement, however I feel this is a great exception of the rule.

I added the entire synopis to all module files and commented out the parts libc++ has not implemented. This makes it easier to enable them once these parts are implemented. Especially when the CI fails it shows a named declaration `std::chrono::ambiguous_local_time` which can be grepped here.

================
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
----------------
EricWF wrote:
> 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.
There are other places in libc++ that use multiple macros to guard them. For example, `fstream` requires both `LIBCXX_ENABLE_FILESYSTEM` and `LIBCXX_ENABLE_LOCALIZATION`. I did the same here based on that precedence. 

================
Comment at: libcxx/src/tz.cpp:107
+  chrono::__skip_optional_whitespace(__input);
+  chrono::__matches(__input, "version");
+  chrono::__skip_mandatory_whitespace(__input);
----------------
EricWF wrote:
> `__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.
> 
> 
We use both ugly and non-uqly naming in our dylib. I prefer ugly; it makes it easy to move code from the dylib to header without the need for uglyfication.

================
Comment at: libcxx/src/tz.cpp:140
+  filesystem::path __root = chrono::__libcpp_tzdb_directory();
+  ifstream __tzdata{__root / "tzdata.zi"};
+  return chrono::__parse_version(__tzdata);
----------------
EricWF wrote:
> 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?
> 
> 
I used that in the past and I was not happy with that code. Note at the moment only a version number is parsed. There is a lot more to be parsed in followup patches,

================
Comment at: libcxx/src/tzdb_list.cpp:42
+
+  const_iterator erase_after(const_iterator __p) {
+#ifndef _LIBCPP_HAS_NO_THREADS
----------------
EricWF wrote:
> 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).
Based on the wording in http://eel.is/c++draft/time.zone.db.list#lib:begin,tzdb_list this matches the wording and its intend. Which part does not make sense to you?

================
Comment at: libcxx/src/tzdb_list.cpp:72
+#ifndef _LIBCPP_HAS_NO_THREADS
+  mutable shared_mutex __mutex_;
+#endif
----------------
EricWF wrote:
> Why is this a shared_mutex? 
> 
> 
The write access is only needed when this object is modified. In practice this should be very rare so read operations should be concurrent.

The main reason for writing is when reloading the TZDB, which requires a new database to be released and updated by the system administrator. New databases happen a handful of times per year, so it's only useful for long running applications.

I'll add a comment to explain this.

================
Comment at: libcxx/src/tzdb_list.cpp:83
+
+_LIBCPP_EXPORTED_FROM_ABI tzdb_list::~tzdb_list() { delete __impl_; }
+
----------------
EricWF wrote:
> This should never be called. 
What do you mean?

================
Comment at: libcxx/test/libcxx/diagnostics/chrono.nodiscard_extensions.compile.pass.cpp:25
+#include "test_macros.h"
+
+void test() {
----------------
EricWF wrote:
> Just make these normal tests and call the functions.
> 
I really don't understand what you mean. Note this test only tests the nodiscard extension nothing else. This is the same of other tests in this directory. There are different tests that test the behaviour of these funtions.

What is the problem with this test?

================
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"
----------------
EricWF wrote:
> So it has complete TZDB?
Can you elaborate what you mean. The TZDB feature is experimental and incomplete. There are several additional patches in the works to get somewhat useful behaviour.

================
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();
+
----------------
EricWF wrote:
> This is a clever way to test the internals. Good thinking.
> 
> Though it feels like this should probably return a string?
Why the returned value is only used for parsing so a temporary is fine. Note that this is only used for testing purposes.

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

+    - label: "No time zone database"
+      command: "libcxx/utils/ci/run-buildbot generic-no-tzdb"
----------------
EricWF wrote:
> Please remove this configuration.
> 
> We don't have the capacity for it.
Since we intend to ship libc++ with TZDB disabled we should test it. The TZDB adds quite some code in its final incarnation. This is useful for embedded systems.

I agree we should look at how we want to do testing. What I would like is something GitLab offers. Make some tests manually. These are not executed by default, but you can start them manually when you want to test them. Next to that nightly builds will enable them automatically.

I think this would apply to several of our parts disabled tests. Most of them are very specific.
I hope we can look at that when Phabricator is retired and we can move our CI to GitHub actions.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154282