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

Sun Aug 21 17:25:52 PDT 2022

philnik added inline comments.

================
Comment at: libcxx/include/__config:1214
 #    define _LIBCPP_PACKED_BYTE_FOR_AIX_END /* empty */
 #  endif

----------------
tahonermann wrote:
> tahonermann wrote:
> > 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.
> I audited the P0482R6 wording and found evidence of declarations for everything except the polymorphic allocator related declarations:
> - `std::pmr::u8string`
> - `std::hash<std::pmr::u8string>`
> 
> This appears to be expected given that [[ https://wg21.link/p0220r1 | P0220R1 ]] is still listed as in-progress in `libcxx/docs/Status/Cxx17Papers.csv` and that https://reviews.llvm.org/D89057 is still awaiting revisions and approvals.
> 
> I think the current state plus this patch suffices to consider P0482R6 complete. I'll update the status page to mark it complete with a note that the missing declarations are pending completion of polymorphic allocator support.
Then we can't consider it complete yet. It would be great though if you could mark it as `Partial` and add a note that only the `std::pmr::u8string` alias and `std::hash<std::pmr::u8string>` are missing.

================
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
----------------
tahonermann wrote:
> 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.
Hmm. Having the declarations in `uchar.h` but not in `cuchar` would indeed be quite weird. OTOH having the declarations pre-C++20 would be an extension in the C library, which we might not want to support. I also suspect that you will find quite strong opposition to enabling `char8_t` pre-C++20. We are quite anti-extensions and have even removed a number of them over the last years. @ldionne Do you have any thoughts here?

================
Comment at: libcxx/include/uchar.h:14
 /*
-    uchar.h synopsis // since C++11
+    uchar.h synopsis // since C11

----------------
tahonermann wrote:
> 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.
Yes, options 2 sounds like the right thing.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130946