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

Fri Mar 17 15:07:09 PDT 2023

ldionne marked 8 inline comments as done.
ldionne added inline comments.

================
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
----------------
Mordante wrote:
> I assume this answers the question of the removal if the macro in classes, right?
I am not certain I understand the question. I went through your comments but I don't remember seeing something related to removal of the macro in classes. Can you clarify?

================
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) {
----------------
Mordante wrote:
> 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.
If it does, then that would be guarded by the availability macros on `std::to_chars(floating-point)` itself. IOW, if this ends up calling something that could be missing from the dylib on your deployment target, it'll be caught because `std::to_chars(floating-point)` is annotated properly.

================
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:
----------------
Mordante wrote:
> These containers can contain `floating-point` values too.
As we discussed: yes, but in that case, the code will fail to compile due to the availability markup on the floating point formatter instantiated within. But using a container adaptor without any floating point in it should work, and it shouldn't fail to compile.

================
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
----------------
Mordante wrote:
> 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`.
We can add a TODO, however it would likely be super long lived since we probably won't remove `_LIBCPP_AVAILABILITY_FORMAT` before a long long time, aka when we drop support for the OS in which those will first be introduced. That'll be years.

However, I think what we should do is fix this problem in Clang, since that's clearly (?) a Clang bug.

================
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");

----------------
Mordante wrote:
> 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.
Since that's a static library, the destructor is effectively compiled into the user's binary, removing the deployment target requirement. Putting it in a static library is as-if it has been defined in the headers from a where-the-code-ends-up-compiled perspective.

================
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 {
----------------
Mordante wrote:
> A tuple may contain a floating-point value.
Same answer as for container adaptors!

================
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;

----------------
Mordante wrote:
> The sequence, map, and set version may be used with floating-point types.
Same answer as for container adaptors!

================
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
----------------
Mordante wrote:
> If it doesn't hurt I prefer to keep this one and remove it in D145853 instead.
I guess I am fine with either, but I don't understand why you'd remove that in D145853. If your concern is about merge conflicts, I am fine with rebasing this change on top of yours. Leaving comment open to determine what to do.

================
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 {
----------------
Mordante wrote:
> `_Tp` may be a floating-point type.
Same answer as for container adaptors!

================
Comment at: libcxx/test/libcxx/utilities/format/format.availability.verify.cpp:15
+// checking.
+// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error
+
----------------
Mordante wrote:
> Just curious, do we have these tests for other availability macros too?
We have a few, for example `libcxx/test/libcxx/thread/barrier.availability.verify.cpp`.

================
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}}
+
----------------
Mordante wrote:
> Is this on your list to investigate?
Yes, I would like to get to it eventually, but I won't lie to you, it'll take me some time.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134598