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

[Mark de Wever via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/docs/Status/Cxx20.rst:54
    .. [#note-P0660] P0660: Section 32.3 Stop Tokens is complete. ``jthread`` hasn't been implemented yet.
+   .. [#note-P0355] P0355: The implementation status is:
+
----------------
philnik wrote:
> Maybe a status page would be appropriate?
I think keeping the note would be better. I expect most of the missing parts done this cycle. The Stream output remark is permanent. That part of the paper will never be done.


================
Comment at: libcxx/docs/UsingLibcxx.rst:577-580
+   std::string_view std::chrono::__libcpp_tzdb_directory() {
+     static std::string_view result = "C:/usr/share/zoneinfo";
+     return result;
+   }
----------------
philnik wrote:
> Why do you make this a static? The string literal already has static storage duration.
I've added a comment to explain the requirement.


================
Comment at: libcxx/include/__chrono/tzdb.h:19
+
+#  include <string>
+
----------------
philnik wrote:
> Including headers shouldn't depend on `-fexperimental-library`. Same for `#pragma GCC system_header`.
This is how we have done this in the past for `format` too.


================
Comment at: libcxx/src/tz.cpp:42
+
+[[nodiscard]] static bool __is_whitespace(int __c) { return __c == ' ' || __c == '\t'; }
+
----------------
philnik wrote:
> Why don't you put this in an anonymous namespace instead? Also, these functions and arguments don't have to be _Uglified, since they are only in the sources. I would also move this into the global namespace to avoid confusion.
I prefer `static` over anonymous namespaces. We have other parts of the dylib uglified too. This makes it easier to move code to the header when possible.


================
Comment at: libcxx/src/tz.cpp:124-140
+_LIBCPP_NODISCARD_EXT _LIBCPP_AVAILABILITY_TZDB _LIBCPP_EXPORTED_FROM_ABI tzdb_list& get_tzdb_list() {
+  static tzdb_list __result{chrono::__make_tzdb()};
+  return __result;
+}
+
+_LIBCPP_AVAILABILITY_TZDB _LIBCPP_EXPORTED_FROM_ABI const tzdb& reload_tzdb() {
+  if (chrono::remote_version() == chrono::get_tzdb().version)
----------------
philnik wrote:
> I would keep all the annotations exclusively in the headers. That makes this code much nicer to read.
I prefer to keep the declaration and definition the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154282