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

Mark de Wever via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Apr 19 11:31:51 PDT 2022


Mordante marked 3 inline comments as done.
Mordante added a comment.

Thanks for the feedback! I'll look into your suggestions.



================
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);
+
----------------
vitaut wrote:
> 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.
Good point about the size of the constants. I'll change this.


================
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;
----------------
vitaut wrote:
> nit: else seems redundant here and everywhere in this function
All but the last are redundant, however I prefer to use `else` in a `if constexpr` since the `else` isn't always redundant.


================
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>();
+};
----------------
vitaut wrote:
> What is this struct for? Why not use `__arg_t` and the "normalized" type directly instead of wrapping in a struct?
The `__arg` indeed seems not needed. But the `__make_storage` is used in the next patch in the series. So it still has its merits.


================
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.
----------------
vitaut wrote:
> 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.
I think I need both for the `basic-format-string` changes. Based on your feedback I'll look at some alternative approaches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121514



More information about the libcxx-commits mailing list