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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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:
> ldionne wrote:
> > tahonermann wrote:
> > > philnik wrote:
> > > > 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?
> > > @ldionne, could you please share your thoughts on whether the `std::c8rtomb()` and `std::mbrtoc8()` declarations should be restricted to C++20 and later or provided consistent with the corresponding C declarations in the global namespace?
> > My preference would be to be strict (and hence not include them before C++20). That being said, it seems like we have a strong precedent everywhere in our C compatibility headers to include functions inside `std::` whenever they are provided by the underlying C library. So I think the current approach is OK.
> > 
> > Is there a reason why we even need `_LIBCPP_HAS_NO_C8RTOMB_MBRTOC8` at all? We have `_LIBCPP_USING_IF_EXIST` to eliminate the need for exactly this sort of stuff. Of course GCC doesn't implement it, however it would still work when compiling using Clang on a platform that uses glibc.
> My first patch relied solely on `_LIBCPP_USING_IF_EXIST`, but that lead to CI build failures as can be seen at https://reviews.llvm.org/harbormaster/unit/view/4733761/. It seems that all such unconditional uses of `_LIBCPP_USING_IF_EXIST` are only exercised in CI builds where the dependent declarations are present. I followed the precedent that appears to be in place for other such problematic cases; see `_LIBCPP_HAS_NO_FGETPOS_FSETPOS` for example.
`_LIBCPP_HAS_NO_FGETPOS_FSETPOS` predates the existence of `_LIBCPP_USING_IF_EXIST`. https://reviews.llvm.org/harbormaster/unit/view/4733761/ is a failure on GCC, which I would indeed expect because GCC does not implement the `using_if_exists` attribute.

I guess my question is: would you see an issue with supporting `<cuchar>` only on Clang, or on GCC when a suitable Glibc is present? Glibc 2.36 is pretty recent, so that would mean only on Clang at least for some time. IMO this is acceptable since libc++'s primary compiler is Clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130946