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

[Victor Zverovich via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/__format/format_arg.h:232-234
+  _LIBCPP_INLINE_VISIBILITY
+  explicit basic_format_arg(nullptr_t) noexcept
+      : __ptr(nullptr), __type_(__format::__arg_t::__ptr) {}
----------------
An overload for `[const] void*` is missing.


================
Comment at: libcxx/include/__format/format_arg.h:238-239
+
+  friend bool operator==(const basic_format_arg& __lhs,
+                         const basic_format_arg& __rhs) {
+    if (__lhs.__type_ != __rhs.__type_)
----------------
In general `basic_format_arg`s are not comparable because they can refer to type-erased user-defined objects/formatters that can't be compared. If this is for testing I suggest moving to tests.


================
Comment at: libcxx/include/__format/format_context.h:63
+  _LIBCPP_INLINE_VISIBILITY
+  basic_format_context(_OutIt __out_it,
+                       basic_format_args<basic_format_context> __args)
----------------
You could expose `__uglified` public "factory" functions for testing. MSVC is not a great example, they were doing a bunch of things wrong, I would reported this one if I had time.


================
Comment at: libcxx/include/__format/format_fwd.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
What is the purpose of the `format_fwd.h` header?


================
Comment at: libcxx/include/format:237
+struct _LIBCPP_TEMPLATE_VIS __format_arg_store {
+  array<basic_format_arg<_Context>, sizeof...(_Args)> __args;
+};
----------------
This could be a built-in array and the dependency on `<array>` removed. There is no advantage to using `std::array` here.


================
Comment at: libcxx/test/std/utilities/format/format.arguments/format.arg.store/class.pass.cpp:35-38
+    static_assert(
+        std::is_same_v<decltype(store.__args),
+                       std::array<std::basic_format_arg<Context>, 0>>);
+    assert(store.__args.size() == 0);
----------------
These checks seem overspecified especially considering that the representation will have to change to implement one of the TODOs. I'd recommend doing checks by converting `__format_arg_store` into `basic_format_args` and using the public API. Same elsewhere.

Another advantage of going through `basic_format_args` is that it will mimic the actual use.


================
Comment at: libcxx/test/std/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp:87-108
+      // char to CharT -> int
+      // This happens when CharT is a char8_t, char16_t, or char32_t and char
+      // is a signed type.
+      // Note if sizeof(CharT) > sizeof(int) this test fails. If there are
+      // platforms where that occurs extra tests need to be added for char32_t
+      // testing it against a long long.
+      test<Context, int, char>('a');
----------------
All of this will have to change when we add `char*_t` support. I think we should static_assert in `basic_format_context` or elsewhere that `CharT` is `char` or `wchar_t` for now since other char type are not usable with `<format>` anyway and these conversions are morally wrong.


================
Comment at: libcxx/test/std/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp:131
+
+  test<Context, std::conditional_t<sizeof(long) == sizeof(int), int, long long>,
+       long>(std::numeric_limits<long>::min());
----------------
Create an alias for `std::conditional_t<sizeof(long) == sizeof(int), int, long long>` to avoid copy-paste?


================
Comment at: libcxx/test/std/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp:197-216
+  test<Context,
+       std::conditional_t<sizeof(unsigned long) == sizeof(unsigned), unsigned,
+                          unsigned long long>,
+       unsigned long>(0);
+  test<Context,
+       std::conditional_t<sizeof(unsigned long) == sizeof(unsigned), unsigned,
+                          unsigned long long>,
----------------
ditto


================
Comment at: libcxx/test/std/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp:280-290
+  test<Context, std::basic_string_view<CharT>>(std::basic_string_view<CharT>());
+  test<Context, std::basic_string_view<CharT>, std::basic_string_view<CharT>>(
+      empty);
+  test<Context, std::basic_string_view<CharT>, std::basic_string_view<CharT>>(
+      str);
+
+  // Test string types.
----------------
Would be nice to add a test for `basic_string_view` with traits and `basic_string` with traits and an allocator.


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.context/arg.pass.cpp:29
+
+  std::basic_string<CharT> str;
+  {
----------------
nit: I'd call it `output` or `result` to distinguish from `string` above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103357