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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
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:
> > 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)`.
> That's actually a great point. I think I can go one step further and remove `_LIBCPP_AVAILABILITY_FORMAT` altogether. If you use something from `<format>` and it ends up using `std::to_chars(floating-point)`, then so be it, you'll get a compiler error. If not then no compiler error. I think that actually makes the most sense.
> 
> If we have a deployment requirement for `std::format` itself, then we can re-add markup.
I agree, removing it altogether was on my mind too, but I wasn't sure whether it was possible, hence the question regarding the floating_point formatter. I assume you intend to remove them everywhere so I will not review this version further. Please let me know if you want me to review this version first.

I think I will move the `format_error` destructor to the header, then we won't need an availability macro. I'm not sure whether that will impact Apple users. But let's discuss that at another time.


================
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:
> > > 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.
> Ok, I reduced and created http://llvm.org/PR61563. I'll link to it.
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134598