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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 14 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__format/format_arg_store.h:157
+  requires(same_as<typename _Context::char_type, wchar_t> && same_as<_CharT, char>)
+consteval auto __make_storage_type() {
+  return __storage_type<wchar_t>{};
----------------
EricWF wrote:
> These overloads are constrained in entirely different ways. Is it possible to use a single method to constrain each of them? 
You mean like using `requires` in all of them?


================
Comment at: libcxx/include/__format/format_arg_store.h:245
+template <class _Context, class _Tp, class... _Args>
+consteval void __get_meta_data(uint32_t __offset, __arg_meta_data* __meta_data) {
+  auto __s = __make_storage_type<_Context, _Tp>();
----------------
EricWF wrote:
> Why do some of these functions take output parameters and other return by value? An overload set should be cohesive in how you use it.
Based on your remarks and the remark of the fold expression I've removed these overloads and I'm using fold expressions instead.


================
Comment at: libcxx/include/format:193
 template <class _Context = format_context, class... _Args>
-_LIBCPP_HIDE_FROM_ABI __format_arg_store<_Context, _Args...>
-make_format_args(const _Args&... __args) {
-  return {basic_format_arg<_Context>(__args)...};
+_LIBCPP_HIDE_FROM_ABI auto make_format_args(const _Args&... __args) {
+  return _VSTD::__make_format_args<_Context>(__args...);
----------------
EricWF wrote:
> This isn't specified as returning auto so you should spell out the return type. Making the compiler deduce it has costs and can potentially break SFINAE.
> 
> 
Normally I agree, but in this case the return type is a bit awkward. The change implements parts of P2418 but not all. After P2418 is implemented it's easy to make it a concrete type as is. (The awkward part is the due to the helper function at line 185.)

I've added a TODO to remind me to change it back.

Same for the next comment.


================
Comment at: libcxx/test/libcxx/utilities/format/format.arguments/format.arg/format_arg_store.pass.cpp:89
+
+  CharT* cstring = const_cast<CharT*>(CSTR("hello C"));
+  const CharT* const_cstring = CSTR("hello const C");
----------------
EricWF wrote:
> This is undefined behavior. Don't
What's exactly UB? The cast is valid, it becomes UB when the test tries to write to `cstring`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121514