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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante planned changes to this revision.
Mordante added a comment.

Thanks for the review. I'll some more time to look at your last suggestion.



================
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) {}
----------------
vitaut wrote:
> An overload for `[const] void*` is missing.
That's the TODO below. I haven't added proper pointer formatting yet. (In hindsight I should also have removed the `nullptr` constructor for now.


================
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_)
----------------
vitaut wrote:
> 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.
It's for testing purposes. However when I originally added this I hadn't implemented the visitor yet. So I can used the visitor in the tests and remove this altogether.


================
Comment at: libcxx/include/__format/format_fwd.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
vitaut wrote:
> What is the purpose of the `format_fwd.h` header?
Adding some declarations for types that need to know the other types exist.
(And slightly abused to declare some parts of the not yet implemented iterator library. That will be removed once libc++ implements these parts of the C++20 iterator library.)


================
Comment at: libcxx/include/format:237
+struct _LIBCPP_TEMPLATE_VIS __format_arg_store {
+  array<basic_format_arg<_Context>, sizeof...(_Args)> __args;
+};
----------------
vitaut wrote:
> This could be a built-in array and the dependency on `<array>` removed. There is no advantage to using `std::array` here.
I did some investigation, but changing this to a built-in array was not trivial. Especially since `sizeof...(_Args)` can be zero. I still need to implement the required size optimizations for `basic_format_args`. So I prefer to tackle these two at the same time.
For now I added a TODO in the code.


================
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);
----------------
vitaut wrote:
> 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.
I think that's a valid point. Most of this test is "identical" to the test `libcxx/test/std/utilities/format/format.arguments/format.args/ctor.pass.cpp`. For now I changed the test to use libc++ specific asserts.

@ldionne What's your opinion of testing exposition only classes? I think the tests were great to have during development, but I agree with @vitaut that it would be better to test this class indirectly by using the public interface.


================
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');
----------------
vitaut wrote:
> 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.
For some place where it's easy I already test for `char*_t` support. I think it would make more sense to restrict that in the formatters. There the Standard is explicit about the supported types. I'll add a note to my todo list to update the formatters.


================
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.
----------------
vitaut wrote:
> Would be nice to add a test for `basic_string_view` with traits and `basic_string` with traits and an allocator.
I really like this suggestion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103357