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

Thu Jul 29 07:00:50 PDT 2021

Mordante marked 15 inline comments as done.
Mordante added inline comments.

================
Comment at: libcxx/docs/Status/Format.rst:42-44
+    * C++23 breaks the ABI with `P2216 <https://wg21.link/P2216>`_.
+      This ABI break will be backported to C++20. Therefore the library will not
       be available on platforms where the ABI break is an issue.
----------------
ldionne wrote:
> I'm not sure this is worth mentioning at all, since we're going to implement https://wg21.link/P2216 and users will never notice. Also, it'll have been implemented by the time we ship `<format>` for the first time.
This was based on the original plan to have an incomplete version in LLVM 13. But after our discussion last week I agree this is no longer useful. I'll remove the entire remark.

================
Comment at: libcxx/include/__format/format_arg.h:37
+// to support compilers with partial C++20 support.
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
+
----------------
ldionne wrote:
> We can remove this now. LLVM 13 has branched, and the oldest compiler we support should support concepts unless I'm mistaken. There will be a similar cleanup in the `<ranges>` code we merged already.
> 
> Note: I need to validate that the oldest compiler we support has support for concepts - I think that would be AppleClang 12, but I'm not sure all versions of it support concepts.
I just tested with a quick hack (D107066) and it seems not all CI nodes support concepts.
https://buildkite.com/llvm-project/libcxx-ci/builds/4579

================
Comment at: libcxx/include/__format/format_arg.h:144
+
+  union {
+    bool __bool;
----------------
vitaut wrote:
> Quuxplusone wrote:
> > vitaut wrote:
> > > ldionne wrote:
> > > > Why aren't we using a `variant` here?
> > > > 
> > > > @vitaut I'm being told you originally requested that Mark moves from `variant` to this `union`, was there a reason? I'd like to avoid going back and forth on this issue.
> > > Not using `variant` allows optimizing the representation for the common case of a small number of formatting arguments: http://eel.is/c++draft/format#args-note-1
> > > 
> > > > Implementations should optimize the representation of basic_format_args for a small number of formatting arguments.
> > > > [Note 1: For example, by storing indices of type alternatives separately from values and packing the former. — end note]
> > This open-coded `__arg_t __type_` member takes only 1 byte; libc++'s <variant> would use a field `__variant_index_t __index_` which takes 4 bytes //unless// `_LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION` is set.
> > See D40210.
> > The Right Answer here might depend on how close we think we are to "ABI v2 by default". Currently that looks like "never". ;)
> > 
> > However however, my understanding of @vitaut's comment is that this code still is not implementing the recommended practice. The recommended practice is that `basic_format_args` shouldn't store an array of `basic_format_arg {union, char}` structs at all; it should store a struct of arrays (one array of unions and one array of chars).
> > 
> > I don't think there's any motivation to optimize the layout of `basic_format_arg` singular. Using `variant` would be fine, except that it would introduce an extra dependency on `<variant>` and probably harm compile times.
> My understanding is that the optimization is planned for a follow-up diff in which case switch to `variant` would have to be undone. cc @Mordante 
Yes the goal is to first have a working implementation where the observable behaviour matches the Standard.
Then this working implementation can be improved. This makes it a lot easier to detect regressions and measure the improvements.
There is a `TODO FMT` so I won't forget about it.

================
Comment at: libcxx/include/__format/format_arg.h:174-177
+  template <__libcpp_signed_integer _Tp>
+  _LIBCPP_HIDE_FROM_ABI explicit basic_format_arg(_Tp __v) noexcept
+      requires(sizeof(_Tp) <= sizeof(int))
+      : __int(__v), __type_(__format::__arg_t::__int) {}
----------------
ldionne wrote:
> Suggestion to reduce the number of overloads:
> 
> ```
> template <class _Tp>
>   requires (__libcpp_signed_integer<_Tp> || __libcpp_unsigned_integer<_Tp>) // might be a better way to do this
> _LIBCPP_HIDE_FROM_ABI explicit basic_format_arg(_Tp __v) noexcept
> {
>   if constexpr (sizeof(_Tp) <= sizeof(int)) { }
>   else if constexpr (...) { }
>   // etc...
> }
> ```
> 
> Basically, this single constructor should handle all the branches in http://eel.is/c++draft/format#arg-5.
I experimented with this approach and I wasn't happy with the result. If feels odd to first require `__libcpp_signed_integer || __libcpp_unsigned_integer` and then in the code test which of the two has been used. Adding `char_type` and `bool` made to code even less readable. Especially when `char_type` is `wchar_t` it accepts `char`, but it doesn't allow narrowing the other way around.
So I reduced this set to 4 constructors
* `bool` unmodified
* `char_type` unmodified
* `__libcpp_signed_integer` all in one overload and then use the `if constexpr(sizeof(_Tp)...`
* `__libcpp_unsigned_integer` like above

That IMO leads to a better balance between removing overloads and having readable code.

================
Comment at: libcxx/include/__format/format_arg.h:220
+  template <class _Traits>
+  _LIBCPP_HIDE_FROM_ABI explicit basic_format_arg(
+      basic_string_view<char_type, _Traits> __s) noexcept
----------------
ldionne wrote:
> IMO I'd rather have a longer line than a funky line break here.
> 
> Suggestion: Put `_LIBCPP_HIDE_FROM_ABI` on a line above, like so:
> 
> ```
> _LIBCPP_HIDE_FROM_ABI
> explicit basic_format_arg(...);
> ```
> 
> This is what we do in most places. As we just talked about, if this creates pain because of clang-format and rebasing, don't waste your time on this. It's just a suggestion.
I tested it. Making this change manually will be reverted by clang-format.

================
Comment at: libcxx/test/libcxx/utilities/format/format.formatter/format.context/locale.pass.cpp:40
+    std::basic_format_context context =
+        std::__format_context_create(out_it, args, en_US);
+    assert(args.__size() == 4);
----------------
ldionne wrote:
> Would it be possible to move those tests into `libcxx/test/std` if we added a function that creates a `basic_format_context` to the test suite? Different implementations could hook into that function (in the test suite) to tell the test suite how to create a `basic_format_context`. Do you think that would make sense and be feasible?
> 
> I'm trying to see how we can test those parts of the API which are kind of implementation details of the library, but at the same time they're defined in the spec.
It's very feasible I just need 2 wrapper functions that call `std::__format_context_create`; one with a `std::locale` and one without that argument not; basically the same as the `std::format*` functions.

Whether it makes sense, that depends on whether or not other vendors will supply patches for their hooks.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103357