Addresses review comments.

Author: mordante

libcxx/include/__chrono/sys_info.h

@@ -28,8 +28,7 @@
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-#  if _LIBCPP_STD_VER >= 20 && !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) &&   \
-      !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+#  if _LIBCPP_STD_VER >= 20
 
 namespace chrono {
 
@@ -43,8 +42,7 @@ struct sys_info {
 
 } // namespace chrono
 
-#  endif // _LIBCPP_STD_VER >= 20 && !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM)
-         // && !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+#  endif //_LIBCPP_STD_VER >= 20
 
 _LIBCPP_END_NAMESPACE_STD
 

libcxx/include/chrono

@@ -893,6 +893,7 @@ constexpr chrono::year                                  operator ""y(unsigned lo
 #include <__chrono/month_weekday.h>
 #include <__chrono/monthday.h>
 #include <__chrono/steady_clock.h>
+#include <__chrono/sys_info.h>
 #include <__chrono/system_clock.h>
 #include <__chrono/time_point.h>
 #include <__chrono/weekday.h>
@@ -918,7 +919,6 @@ constexpr chrono::year                                  operator ""y(unsigned lo
 #if !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) &&                              \
     !defined(_LIBCPP_HAS_NO_LOCALIZATION)
 #  include <__chrono/leap_second.h>
-#  include <__chrono/sys_info.h>
 #  include <__chrono/time_zone.h>
 #  include <__chrono/time_zone_link.h>
 #  include <__chrono/tzdb.h>

libcxx/src/include/tzdb/time_zone_private.h

@@ -24,7 +24,8 @@ namespace chrono {
 
 class time_zone::__impl {
 public:
-  explicit _LIBCPP_HIDE_FROM_ABI __impl(string&& __name) : __name_(std::move(__name)) {}
+  explicit _LIBCPP_HIDE_FROM_ABI __impl(string&& __name, const __tz::__rules_storage_type& __rules_db)
+      : __name_(std::move(__name)), __rules_db_(__rules_db) {}
 
   [[nodiscard]] _LIBCPP_HIDE_FROM_ABI string_view __name() const noexcept { return __name_; }
 
@@ -33,12 +34,20 @@ class time_zone::__impl {
     return __continuations_;
   }
 
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI const __tz::__rules_storage_type& __rules_db() const { return __rules_db_; }
+
 private:
   string __name_;
   // Note the first line has a name + __continuation, the other lines
   // are just __continuations. So there is always at least one item in
   // the vector.
   vector<__tz::__continuation> __continuations_;
+
+  // Continuations often depend on a set of rules. The rules are stored in
+  // parallel data structurs in tzdb_list. From the time_zone it's not possible
+  // to find its associated tzdb entry and thus not possible to find its
+  // associated rules. Therefore a link to the rules in stored in this class.
+  const __tz::__rules_storage_type& __rules_db_;
 };
 
 } // namespace chrono

libcxx/src/include/tzdb/tzdb_list_private.h

@@ -81,13 +81,6 @@ class tzdb_list::__impl {
     return __tzdb_.end();
   }
 
-  const __tz::__rules_storage_type& __rules() const noexcept {
-#ifndef _LIBCPP_HAS_NO_THREADS
-    shared_lock __lock{__mutex_};
-#endif
-    return __rules_.front();
-  }
-
 private:
   // Loads the tzdbs
   // pre: The caller ensures the locking, if needed, is done.

libcxx/src/time_zone.cpp

@@ -17,7 +17,7 @@
 // It would be possible to cache lookups. If a time for a zone is calculated its
 // sys_info could be kept and the next lookup could test whether the time is in
 // a "known" sys_info. The wording in the Standard hints at this slowness by
-// "suggesting" this could be implemented at the user's side.
+// "suggesting" this could be implemented on the user's side.
 
 // TODO TZDB look at removing quirks
 //
@@ -30,6 +30,7 @@
 // which implies there are no sys_info objects with a duration of less than 12h.
 
 #include <algorithm>
+#include <cctype>
 #include <chrono>
 #include <expected>
 #include <map>
@@ -111,20 +112,8 @@ __binary_find(_Range&& __r, const _Type& __value, _Comp __comp = {}, _Proj __pro
 //    text in the appropriate Rule's LETTER column, and the resulting string
 //    should be a time zone abbreviation
 //
-// Accepting invalid formats that can be processed in a sensible way would better
-// serve the user than throwing an exception. So some of these rules are not
-// strictly validated.
-// 1  This is not validated. Some examples that will be accepted are, "+04:30",
-//    "Q", "42".
-// 2  How this format is formatted is not specified. In the current tzdata.zi
-//    this value is not used. This value is accepted in a part of the format. So
-//    "a%s%zb" will be considered valid.
-// 3  This is not validated, the output might be incorrect.
-//    Proper validation would make the algorithm more complex. Then the first
-//    element of the pair is used the parsing of FORMAT can stop. To do proper
-//    validation the tail should be validated.
-// 4  This value is accepted in a part of the format. So "a%s%zb" will be
-//    considered valid.
+// Rule 1 is not strictly validated since America/Barbados uses a two letter
+// abbreviation AT.
 [[nodiscard]] static string
 __format(const __tz::__continuation& __continuation, const string& __letters, seconds __save) {
   bool __shift = false;
@@ -137,6 +126,11 @@ __format(const __tz::__continuation& __continuation, const string& __letters, se
         break;
 
       case 'z': {
+        if (__continuation.__format.size() != 2)
+          std::__throw_runtime_error(
+              std::format("corrupt tzdb FORMAT field: %z should be the entire contents, instead contains '{}'",
+                          __continuation.__format)
+                  .c_str());
         chrono::hh_mm_ss __offset{__continuation.__stdoff + __save};
         if (__offset.is_negative()) {
           __result += '-';
@@ -164,14 +158,22 @@ __format(const __tz::__continuation& __continuation, const string& __letters, se
 
     } else if (__c == '%') {
       __shift = true;
-    } else {
+    } else if (__c == '+' || __c == '-' || std::isalnum(__c)) {
       __result.push_back(__c);
+    } else {
+      std::__throw_runtime_error(
+          std::format(
+              "corrupt tzdb FORMAT field: invalid character '{}' found, expected +, -, or an alphanumeric value", __c)
+              .c_str());
     }
   }
 
   if (__shift)
     std::__throw_runtime_error("corrupt tzdb FORMAT field: input ended with the start of the escape sequence '%'");
 
+  if (__result.empty())
+    std::__throw_runtime_error("corrupt tzdb FORMAT field: result is empty");
+
   return __result;
 }
 
@@ -300,11 +302,10 @@ class __named_rule_until {
   return chrono::__from_to_sys_seconds(__stdoff, __rule, __rule.__from);
 }
 
-[[nodiscard]] static const vector<__tz::__rule>& __get_rules(const string& __rule_name) {
-  const __tz::__rules_storage_type& __rules = get_tzdb_list().__implementation().__rules();
-
-  auto __result = chrono::__binary_find(__rules, __rule_name, {}, [](const auto& __p) { return __p.first; });
-  if (__result == std::end(__rules))
+[[nodiscard]] static const vector<__tz::__rule>&
+__get_rules(const __tz::__rules_storage_type& __rules_db, const string& __rule_name) {
+  auto __result = chrono::__binary_find(__rules_db, __rule_name, {}, [](const auto& __p) { return __p.first; });
+  if (__result == std::end(__rules_db))
     std::__throw_runtime_error(("corrupt tzdb: rule '" + __rule_name + " 'does not exist").c_str());
 
   return __result->second;
@@ -348,7 +349,7 @@ class __named_rule_until {
 //   R HK 1946 o - Ap 21 0 1 S  // (3)
 // There (1) is active until Novemer 18th 1945 at 02:00, after this time
 // (2) becomes active. The first rule entry for HK (3) becomes active
-// from pril 21st 1945 at 01:00. In the period between (2) is active.
+// from April 21st 1945 at 01:00. In the period between (2) is active.
 // This entry has an offset.
 // This entry has no save, letters, or dst flag. So in the period
 // after (1) and until (3) no rule entry is associated with the time.
@@ -439,11 +440,11 @@ __next_rule(sys_seconds __time,
       if (__y == __year && __it == __current)
         continue;
 
-      sys_seconds __t = __rule_to_sys_seconds(__stdoff, __save, *__it, __y);
+      sys_seconds __t = chrono::__rule_to_sys_seconds(__stdoff, __save, *__it, __y);
       if (__t <= __time)
         continue;
 
-      _LIBCPP_ASSERT(!__candidates.contains(__t), "duplicated rule");
+      _LIBCPP_ASSERT_INTERNAL(!__candidates.contains(__t), "duplicated rule");
       __candidates[__t] = __it;
       break;
     }
@@ -491,11 +492,9 @@ __first_rule(seconds __stdoff, const vector<__tz::__rule>& __rules) {
     sys_seconds __time,
     sys_seconds __continuation_begin,
     const __tz::__continuation& __continuation,
-    const string& __rule_name) {
-  const vector<__tz::__rule>& __rules = __get_rules(__rule_name);
-
+    const vector<__tz::__rule>& __rules) {
   auto __rule = chrono::__first_rule(__continuation.__stdoff, __rules);
-  _LIBCPP_ASSERT(__rule != __rules.end(), "the set of rules has no first rule");
+  _LIBCPP_ASSERT_INTERNAL(__rule != __rules.end(), "the set of rules has no first rule");
 
   // Avoid selecting a time before the start of the continuation
   __time = std::max(__time, __continuation_begin);
@@ -662,12 +661,16 @@ __first_rule(seconds __stdoff, const vector<__tz::__rule>& __rules) {
 }
 
 [[nodiscard]] static __sys_info_result
-__get_sys_info(sys_seconds __time, sys_seconds __continuation_begin, const __tz::__continuation& __continuation) {
+__get_sys_info(sys_seconds __time,
+               sys_seconds __continuation_begin,
+               const __tz::__continuation& __continuation,
+               const __tz::__rules_storage_type& __rules_db) {
   return std::visit(
       [&](const auto& __value) {
         using _Tp = decay_t<decltype(__value)>;
         if constexpr (same_as<_Tp, std::string>)
-          return chrono::__get_sys_info_rule(__time, __continuation_begin, __continuation, __value);
+          return chrono::__get_sys_info_rule(
+              __time, __continuation_begin, __continuation, __get_rules(__rules_db, __value));
         else if constexpr (same_as<_Tp, monostate>)
           return chrono::__get_sys_info_basic(__time, __continuation_begin, __continuation, chrono::seconds(0));
         else if constexpr (same_as<_Tp, __tz::__save>)
@@ -726,7 +729,7 @@ _LIBCPP_EXPORTED_FROM_ABI time_zone::~time_zone() = default;
 time_zone::__get_info(sys_seconds __time) const {
   optional<sys_info> __result;
   bool __valid_result = false; // true iff __result.has_value() is true and
-                               // result.begin <= __time < __result.end is true.
+                               // __result.begin <= __time < __result.end is true.
   bool __can_merge                 = false;
   sys_seconds __continuation_begin = sys_seconds::min();
   // Iterates over the Zone entry and its continuations. Internally the Zone
@@ -746,9 +749,9 @@ time_zone::__get_info(sys_seconds __time) const {
   // no continuation is applicable it will return the end time as "error". When
   // two continuations are contiguous and contain the "same" information these
   // ranges are merged as one range.
-  // The merging requires to keep results occur before __time, likewise when a
-  // valid result is found the algorithm needs test the next continuation to see
-  // when it can be merged. For example, Africa/Ceuta
+  // The merging requires keeping any result that occurs before __time,
+  // likewise when a valid result is found the algorithm needs to test the next
+  // continuation to see whether it can be merged. For example, Africa/Ceuta
   // Continuations
   //  0 s WE%sT 1929                   (C1)
   //  0 - WET 1967                     (C2)
@@ -773,13 +776,14 @@ time_zone::__get_info(sys_seconds __time) const {
   // means the period [1928-10-07 00:00:00, 1967-06-03 12:00:00) should be
   // returned in one sys_info object.
 
-  const auto& __continuations = __impl_->__continuations();
+  const auto& __continuations                  = __impl_->__continuations();
+  const __tz::__rules_storage_type& __rules_db = __impl_->__rules_db();
   for (auto __it = __continuations.begin(); __it != __continuations.end(); ++__it) {
     const auto& __continuation   = *__it;
-    __sys_info_result __sys_info = chrono::__get_sys_info(__time, __continuation_begin, __continuation);
+    __sys_info_result __sys_info = chrono::__get_sys_info(__time, __continuation_begin, __continuation, __rules_db);
 
     if (__sys_info) {
-      _LIBCPP_ASSERT(__sys_info->__info.begin < __sys_info->__info.end, "invalid sys_info range");
+      _LIBCPP_ASSERT_INTERNAL(__sys_info->__info.begin < __sys_info->__info.end, "invalid sys_info range");
 
       // Filters out dummy entries
       // Z America/Argentina/Buenos_Aires -3:53:48 - LMT 1894 O 31
@@ -813,7 +817,8 @@ time_zone::__get_info(sys_seconds __time) const {
         __can_merge    = __sys_info->__can_merge;
       } else if (__can_merge && chrono::__merge_continuation(*__result, __sys_info->__info)) {
         // The results are merged, update the result state. This may
-        // "overwrite" valid with valid.
+        // "overwrite" a valid sys_info object with another valid sys_info
+        // object.
         __valid_result = __time >= __result->begin && __time < __result->end;
         __can_merge    = __sys_info->__can_merge;
       } else {
@@ -843,7 +848,7 @@ time_zone::__get_info(sys_seconds __time) const {
         if (__valid_result) {
           return *__result;
         } else {
-          _LIBCPP_ASSERT(__it != __continuations.begin(), "the first rule should always seed the result");
+          _LIBCPP_ASSERT_INTERNAL(__it != __continuations.begin(), "the first rule should always seed the result");
           const auto& __last = *(__it - 1);
           if (std::holds_alternative<string>(__last.__rules)) {
             // Europe/Berlin

libcxx/src/tzdb.cpp

@@ -561,7 +561,7 @@ static void __parse_rule(tzdb& __tzdb, __tz::__rules_storage_type& __rules, istr
 
 static void __parse_zone(tzdb& __tzdb, __tz::__rules_storage_type& __rules, istream& __input) {
   chrono::__skip_mandatory_whitespace(__input);
-  auto __p                                      = std::make_unique<time_zone::__impl>(chrono::__parse_string(__input));
+  auto __p                                      = std::make_unique<time_zone::__impl>(chrono::__parse_string(__input), __rules);
   vector<__tz::__continuation>& __continuations = __p->__continuations();
   chrono::__skip_mandatory_whitespace(__input);