[libcxx-commits] [PATCH] D134598: [libc++] Fix availability markup for std::format

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for picking this up! I have a few questions regarding formatters that could be used with floating-point types. I really wonder whether removing their availability macros is correct.



================
Comment at: libcxx/include/__availability:266
+    // those for the time being. When we make it non-experimental, we may decide to put some definitions
+    // in the dylib in which case this deployment target requirement may increase.
+#   define _LIBCPP_AVAILABILITY_FORMAT _LIBCPP_AVAILABILITY_TO_CHARS_FLOATING_POINT
----------------
I assume this answers the question of the removal if the macro in classes, right?


================
Comment at: libcxx/include/__chrono/ostream.h:96
 template <class _CharT, class _Traits, class _Rep, class _Period>
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_AVAILABILITY_FORMAT basic_ostream<_CharT, _Traits>&
+_LIBCPP_HIDE_FROM_ABI basic_ostream<_CharT, _Traits>&
 operator<<(basic_ostream<_CharT, _Traits>& __os, const duration<_Rep, _Period>& __d) {
----------------
Depending on the answer regarding the usage of `floating-point` in general I need to check whether this one can call `to_chars(floating-point)`. The `_Rep` can be a floating-point type, the same may apply to a few other chrono formatters.


================
Comment at: libcxx/include/__format/container_adaptor.h:40
 template <class _Adaptor, class _CharT>
-struct _LIBCPP_TEMPLATE_VIS _LIBCPP_AVAILABILITY_FORMAT __formatter_container_adaptor {
+struct _LIBCPP_TEMPLATE_VIS __formatter_container_adaptor {
 private:
----------------
These containers can contain `floating-point` values too.


================
Comment at: libcxx/include/__format/format_functions.h:441
 
+template <class = void> // This needs to be a template for availability markup not to fire too eagerly
 _LIBCPP_ALWAYS_INLINE inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_AVAILABILITY_FORMAT string
----------------
ldionne wrote:
> I want to call this out -- this is terrible but omitting this creates huge problems with the attribute. I am not convinced this is acceptable.
Interesting. I guess that explains why I had to use the availability macro at a lot of places.

Can we add a `TODO FMT Make non-templated when removing _LIBCPP_AVAILABILITY_FORMAT`.


================
Comment at: libcxx/include/__format/format_parse_context.h:58
     if (__indexing_ == __manual)
       std::__throw_format_error("Using automatic argument numbering in manual argument numbering mode");
 
----------------
Is the removal of `_LIBCPP_AVAILABILITY_FORMAT` really correct since the `format_error`'s destructor in experimental library. I haven't looked whether `basic_format_arg` can throw. I know for certain `formatter::parse` can throw.


================
Comment at: libcxx/include/__format/formatter_tuple.h:42
 template <__fmt_char_type _CharT, class _Tuple, formattable<_CharT>... _Args>
-struct _LIBCPP_TEMPLATE_VIS _LIBCPP_AVAILABILITY_FORMAT __formatter_tuple {
+struct _LIBCPP_TEMPLATE_VIS __formatter_tuple {
   _LIBCPP_HIDE_FROM_ABI constexpr void set_separator(basic_string_view<_CharT> __separator) noexcept {
----------------
A tuple may contain a floating-point value.


================
Comment at: libcxx/include/__format/range_default_formatter.h:87
 template <range_format _Kp, ranges::input_range _Rp, class _CharT>
-struct _LIBCPP_TEMPLATE_VIS _LIBCPP_AVAILABILITY_FORMAT __range_default_formatter;
+struct _LIBCPP_TEMPLATE_VIS __range_default_formatter;
 
----------------
The sequence, map, and set version may be used with floating-point types.


================
Comment at: libcxx/include/__format/range_default_formatter.h:171
   requires(_Kp == range_format::string || _Kp == range_format::debug_string)
-struct _LIBCPP_TEMPLATE_VIS _LIBCPP_AVAILABILITY_FORMAT __range_default_formatter<_Kp, _Rp, _CharT> {
+struct _LIBCPP_TEMPLATE_VIS __range_default_formatter<_Kp, _Rp, _CharT> {
   __range_default_formatter() = delete; // TODO FMT Implement
----------------
If it doesn't hurt I prefer to keep this one and remove it in D145853 instead.


================
Comment at: libcxx/include/__format/range_formatter.h:43
   requires same_as<remove_cvref_t<_Tp>, _Tp> && formattable<_Tp, _CharT>
-struct _LIBCPP_TEMPLATE_VIS _LIBCPP_AVAILABILITY_FORMAT range_formatter {
+struct _LIBCPP_TEMPLATE_VIS range_formatter {
   _LIBCPP_HIDE_FROM_ABI constexpr void set_separator(basic_string_view<_CharT> __separator) noexcept {
----------------
`_Tp` may be a floating-point type.


================
Comment at: libcxx/test/libcxx/utilities/format/format.availability.verify.cpp:15
+// checking.
+// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error
+
----------------
Just curious, do we have these tests for other availability macros too?


================
Comment at: libcxx/test/std/time/time.syn/formatter.month.pass.cpp:16
+// This test requires std::to_chars(floating-point), which is in the dylib
+// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx{{10.9|10.10|10.11|10.12|10.13|10.14|10.15|11.0}}
+
----------------
Is this on your list to investigate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134598