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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
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.
----------------
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.


================
Comment at: libcxx/include/__format/format_arg.h:37
+// to support compilers with partial C++20 support.
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
+
----------------
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.


================
Comment at: libcxx/include/__format/format_arg.h:144
+
+  union {
+    bool __bool;
----------------
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.


================
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) {}
----------------
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.


================
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
----------------
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.


================
Comment at: libcxx/include/__format/format_args.h:56
+
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI size_t __size() const noexcept {
+    return __size_;
----------------
Is this `[[nodiscard]]` really adding anything? Non blocking.

IMO, using `nodiscard` everywhere because C++ has the wrong default here just adds syntactic overhead and reduces the impact of seeing `[[nodiscard]]` on a method that could truly be used incorrectly. I think we should stick to putting it on key methods where we really want to diagnose if the user discards the result, because that would most likely be a bug (e.g. anything called `empty`, something like `lock_guard`, etc).


================
Comment at: libcxx/include/__format/format_context.h:122
+  _LIBCPP_HIDE_FROM_ABI
+  basic_format_context(_OutIt __out_it,
+                       basic_format_args<basic_format_context> __args,
----------------
Maybe those constructors should all be `explicit`?


================
Comment at: libcxx/include/__format/format_fwd.h:39
+template <class _Ip, class _Tp>
+concept __output_iterator = input_or_output_iterator<_Ip> &&
+    indirectly_writable<_Ip, _Tp> && requires(_Ip __i, _Tp&& __t) {
----------------
This has been implemented now.


================
Comment at: libcxx/include/format:13-20
+// TODO FMT Remove this once the library is complete.
+/*
+ * WARNING
+ * The header is work in progress and not complete yet. The ABI isn't stable.
+ * Partly due to the lack of recommended optimizations, partly due to the
+ * current implementation not implementing:
+ * https://wg21.link/P2216 std::format improvements
----------------
This is not required. We're not shipping the library until it's stable anyway, with the mechanism you added.


================
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);
----------------
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.


================
Comment at: libcxx/test/support/test_basic_format_arg.h:15
+template <class Context, class T>
+[[nodiscard]] bool test_basic_format_arg(std::basic_format_arg<Context> arg,
+                                         T expected) {
----------------
Please drop `[[nodiscard]]` in the tests unless it adds a clear benefit (e.g. a method that could meaningfully be misused).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103357