[libcxx-commits] [PATCH] D93593: [libc++][format] Add __format_arg_store.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added a comment.

In D93593#2577296 <https://reviews.llvm.org/D93593#2577296>, @miscco wrote:

> Thanks for working on this. It is a considerable library
>
> I am really concerned about doing a "quick" implementation and then being locked in due to ABI issues.
>
> I believe we should spend the time up front to get a clean efficient implementation

In general I agree. However the ABI will most likely be broken. Victor contacted me and http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2216r2.html will most likely be accepted for C++23 and retroactively be part of C++20. I'll will discuss this on Slack in the near future.

I also like to have some of the basics in so I can add benchmarks to see whether optimizations have a positive effect.

I already added a warning in the release document that the ABI isn't stable yet. Until it is I don't want to set `__cpp_lib_format`.

What do you think @ldionne?

In D93593#2577315 <https://reviews.llvm.org/D93593#2577315>, @tschuett wrote:

> You could put a comment at the top of the format header:
> This library is still in development and there are no stability guarantees.

That sounds like a good idea.

> Or if you dare:
>
> #warning the format library is still in development

I'm quite sure that will fail in our lit tests ;-)



================
Comment at: libcxx/include/format:408-418
+  template <class _Tp>
+  _LIBCPP_INLINE_VISIBILITY explicit basic_format_arg(
+      const _Tp& __value) noexcept
+      requires(_VSTD::is_same_v<_Tp, bool> || _VSTD::is_same_v<_Tp, char_type>
+#ifndef _LIBCPP_HAS_NO_INT128
+               || _VSTD::is_same_v<_Tp, __int128_t> ||
+               _VSTD::is_same_v<_Tp, __uint128_t>
----------------
cjdb wrote:
> miscco wrote:
> > I think we whould really constraint this on `template<integral _Tp>` and handle all integral types here. Having them as multiple separat functions has significant maintainance overhead. Same for `<template<floating_point _Tp>`
> > 
> > I think they are in flight by @cjdb so maybe wait a bit to get this in.
> > 
> > (FYI this i how it I dit it for MSVC STL https://github.com/microsoft/STL/blob/99e8a2d7f2a083a6f768cff35a5e1792feb625a6/stl/inc/format#L933-L962 )
> `same_as`, `integral`, and `floating_point` are all merged :-)
> I think we whould really constraint this on `template<integral _Tp>` and handle all integral types here. Having them as multiple separat functions has significant maintainance overhead. Same for `<template<floating_point _Tp>`

I'll have a look at we can combine. But I think here we need some separate functions. 

> I think they are in flight by @cjdb so maybe wait a bit to get this in.

Yes I've already used some of the newer concepts recently added. But it seems I've missed `same_as`.
 
> (FYI this i how it I dit it for MSVC STL https://github.com/microsoft/STL/blob/99e8a2d7f2a083a6f768cff35a5e1792feb625a6/stl/inc/format#L933-L962 )

Thanks for the link. It seems MSVC has more overloads for this part https://github.com/microsoft/STL/blob/99e8a2d7f2a083a6f768cff35a5e1792feb625a6/stl/inc/format#L195
(Obviously it's required there since Microsoft doesn't use a variant.)



================
Comment at: libcxx/include/format:503-506
+  friend bool operator==(const basic_format_arg& __lhs,
+                         const basic_format_arg& __rhs) {
+    return __lhs.__value_ == __rhs.__value_;
+  }
----------------
miscco wrote:
> I *think* we should be able to default this as it is C++20
I think that should be possible in theory. However I'm not sure how well compiler support for this feature is. Clang hasn't implemented all related papers yet. Since it's trivial to write the function manually I've done so. I'm not sure whether the performance of the variant is good enough. If not I need to change it to something else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93593