[libcxx-commits] [PATCH] D121514: [libc++][format] Improve format-arg-store.

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut requested changes to this revision.
vitaut added a comment.
This revision now requires changes to proceed.

Looks much better, thanks. A few comments inline.



================
Comment at: libcxx/include/__format/format_arg.h:78-79
+
+  if (__id < __packed_types_max)
+    __types >>= __packed_arg_t_bits * (__packed_types_max - 1 - __id);
+
----------------
Do I miss something or you reverse the order of arguments i.e. the first argument is stored in higher bits? Why? This makes the constants bigger and logic slightly more awkward for no good reason.


================
Comment at: libcxx/include/__format/format_arg_store.h:44
+    return __arg_t::__boolean;
+  else if constexpr (same_as<_Tp, char> _LIBCPP_IF_WIDE_CHARACTERS(|| same_as<_Tp, wchar_t>))
+    return __arg_t::__char_type;
----------------
nit: else seems redundant here and everywhere in this function


================
Comment at: libcxx/include/__format/format_arg_store.h:79-83
+template <class _Tp>
+struct __storage_type {
+  using type = _Tp;
+  static constexpr __arg_t __arg = __get_arg_t<_Tp>();
+};
----------------
What is this struct for? Why not use `__arg_t` and the "normalized" type directly instead of wrapping in a struct?


================
Comment at: libcxx/include/__format/format_arg_store.h:202
+_LIBCPP_HIDE_FROM_ABI basic_format_arg<_Context> __create_format_arg(_Tp&& __value) noexcept {
+  using _Sp = typename decltype(__make_storage_type<_Context, remove_cvref_t<_Tp>>())::type;
+  if constexpr (same_as<_Sp, basic_string_view<typename _Context::char_type>>)
----------------
Looks like this is the only usage of `__make_storage_type` which seems weird because only `__storage_type::type` is used. This suggests that instead of returning `__storage_type` `__make_storage_type` better return the "mapped" type itself and possibly be renamed. Perhaps "normalize" is a better name because essentially what this function does is normalizes the type by stripping off traits and mapping numeric types into canonical form.


================
Comment at: libcxx/include/__format/format_arg_store.h:203-225
+  if constexpr (same_as<_Sp, basic_string_view<typename _Context::char_type>>)
+    // When the _Traits or _Allocator are different an implicit conversion will
+    // fail. Instead of adding special cases to __storage<_Sp> handle the
+    // special case here.
+    //
+    // Note since the input can be an array use the non-member functions to
+    // extract the constructor arguments.
----------------
I think it would be cleaner to move this into the function that does normalization by making it take an argument instead of splitting type and value handling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121514