[libcxx-commits] [PATCH] D130946: [libc++][cuchar] Declare std::c8rtomb and std::mbrtoc8 in <cuchar> if available.

[Tom Honermann via Phabricator via libcxx-commits]

tahonermann added a comment.

Thank you for the review @philnik! I'll post an update addressing some of the comments shortly. Responses included for other comments.



================
Comment at: libcxx/include/__config:1214
 #    define _LIBCPP_PACKED_BYTE_FOR_AIX_END /* empty */
 #  endif
 
----------------
philnik wrote:
> Does this complete the implementation of P0482R6? If yes, please update the status page and feature test macros. Otherwise, what is still missing?
I expect that this does complete [[ https://wg21.link/p0482r6 | P0482R6 ]], but I'll double check, report back, and update the status page accordingly.

With regard to updating feature test macros, I don't think there is anything to be done. P0482R6 only specifies the single `__cpp_lib_char8_t` feature test macro for the library. libc++ already defines that macro based on the prior P0482R6 implementation efforts and the recent [[ https://reviews.llvm.org/D129195 | implementation of P1423 ]] bumped its value. Though P0482R6 is not explicit about this (either in prose or wording), it can be inferred that the feature test macro is not applicable to these declarations as it is not required to be defined by the `<cuchar>` or `uchar.h` headers. Additionally, since `c8rtomb()` and `mbrtoc8()` are expected to be provided by the C library, entangling the macro definition with the availability of those declarations would get messy. This is consistent with the behavior of libstdcxx.


================
Comment at: libcxx/include/cuchar:54-57
+#  if !defined(_LIBCPP_HAS_NO_C8RTOMB_MBRTOC8)
+using ::mbrtoc8 _LIBCPP_USING_IF_EXISTS;
+using ::c8rtomb _LIBCPP_USING_IF_EXISTS;
+#  endif
----------------
philnik wrote:
> I think this should also be guarded with `_LIBCPP_STD_VER >= 20`.
That is an option, but leads to scenarios where the declarations are available in the global namespace via `uchar.h` but not in the `std` namespace via `<cuchar>`. I thought it made more sense to keep these in sync.

At present, libc++ omits the other `char8_t` related declarations (e.g., `u8string`) in C++20 mode when `char8_t` support is disabled (of course) but likewise omits them in pre-C++20 modes when `char8_t` support is enabled. What I would prefer to do (in a separate patch) is to match the libstdcxx behavior of declaring all of the `char8_t` related declarations based on `char8_t` enablement rather than on the C++ standard version. This would give users more flexibility for evaluating  and addressing`char8_t` related migration concerns before migrating to C++20.


================
Comment at: libcxx/include/uchar.h:14
 /*
-    uchar.h synopsis // since C++11
+    uchar.h synopsis // since C11
 
----------------
philnik wrote:
> As a C++ implementation we are interested in which C++ version something got added. So please use that version instead of in which C version something got added. Same for the rest of the synopsis.
Things get kind of messy here. The declarations in `<cuchar>` were added in C++20. Since C++20 depends on C17, no version of C++ will require the `uchar.h` declarations until a future C++ standard that is based on C23 (which will be required by ISO to happen for C++26). Our choices are:
1) Omit these declarations pending a future C++ standard.
2) Annotate these declarations as "since C++20" since that will reflect existing practice (e.g., glibc will declare them in `uchar.h` in C++20 mode) and the declarations are needed to satisfy the `std` namespace declarations in `<cuchar>`.
3) Optimistically annotate these as "since C++26" or "expected in C++26".

I'm inclined toward option 2.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130946