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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

This really starts to look nice, there are a few minor issues. After that I want to have look at the final patch before approving.



================
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
----------------
ldionne wrote:
> 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?
Mainly why we can now remove all AVAILABILITY macros in the classes. I think it's mainly a conclusion for me since you answered the other questions.


================
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:
----------------
ldionne wrote:
> 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.
I think the only thing that puzzles my then is why not remove the `_LIBCPP_AVAILABILITY_FORMAT` from `libcxx/include/__format/formatter_floating_point.h` and and let that use transitive depend on `to_char(floating-point)`.


================
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:
> 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.
That makes me want a TODO FMT even more and then a link to the Clang bug-report or the rdar number.


================
Comment at: libcxx/test/libcxx/utilities/format/format.availability.verify.cpp:15
+// checking.
+// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error
+
----------------
ldionne wrote:
> 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`.
Thanks, I hadn't noticed these before.


================
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}}
+
----------------
ldionne wrote:
> 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.
It would be interesting to know what you found thusfar, maybe discuss it on our next 1:1. I don't see this part as a blocker.

I would like to see TODO FMT instead of just TODO. This is consistent with the other format related TODOs.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134598