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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM!



================
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:
> > > > 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.
> In the current state of this patch, all `_LIBCPP_AVAILABILITY_FORMAT` should be gone. I think I would like to get this one reviewed and landed and then I will rebase the other patches in the stack on top of it. Does that make sense?
Yes that sounds good. I just wanted to avoid spending time reviewing, with the chance the patch would be reworked again. The current patch is indeed what I expected the patch would look like :-)


================
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
----------------
ldionne wrote:
> 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.
It would prevent a merge conflict, but since you remove the macro from `__availability` I will have to deal with the conflict.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134598