[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 a comment.
This revision now requires changes to proceed.

"the function make_format_args," appears twice in the summary. I guess the second one was supposed to be "make_wformat_args".



================
Comment at: libcxx/include/__format/format_arg.h:17
+#include <concepts>
+#include <functional>
+#include <string>
----------------
Do we have to include all of functional for invoke?


================
Comment at: libcxx/include/__format/format_arg.h:69
+  case __format::__arg_t::__none:
+    __throw_format_error("Argument index out of bounds");
+  case __format::__arg_t::__bool:
----------------
Visiting an empty argument should pass `monostate` to the visitor, not throw an exception.


================
Comment at: libcxx/include/__format/format_arg.h:100
+        _VSTD::forward<_Visitor>(__vis),
+        static_cast<const typename _Context::char_type*>(__arg.__ptr));
+  case __format::__arg_t::__string_view:
----------------
Why store a C string as `void*` and cast? I suggest storing as `const char_type*` instead.


================
Comment at: libcxx/include/__format/format_arg.h:116
+  _LIBCPP_INLINE_VISIBILITY basic_format_arg() noexcept
+      : __bool{false}, __type_{__format::__arg_t::__none} {}
+
----------------
Initializing `__bool` is unnecessary because the value is never accessed if type is `__none`. Probably doesn't matters in practice but there is no harm is doing less unnecessary work.


================
Comment at: libcxx/include/__format/format_context.h:51-62
+  // Note: the Standard doesn't specify the required constructors.
+  _LIBCPP_INLINE_VISIBILITY
+  basic_format_context(_OutIt __out_it,
+                       basic_format_args<basic_format_context> __args,
+                       optional<_VSTD::locale>&& __loc = nullopt)
+      : __out_it_(_VSTD::move(__out_it)), __args_(__args),
+        __loc_(_VSTD::move(__loc)) {}
----------------
`basic_format_context` is not supposed to be constructed by users so these ctors better be private.


================
Comment at: libcxx/include/__format/format_context.h:92
+  // This is done by storing the locale of the constructor in this optional. If
+  // locale() is called and the optional has no value the value will be create.
+  // This allow the implementation to lazily create the locale.
----------------
create -> created


================
Comment at: libcxx/include/__format/format_context.h:93
+  // locale() is called and the optional has no value the value will be create.
+  // This allow the implementation to lazily create the locale.
+  // TODO FMT Validate whether lazy creation is the best solution.
----------------
allow -> allows


================
Comment at: libcxx/include/__format/format_context.h:94-95
+  // This allow the implementation to lazily create the locale.
+  // TODO FMT Validate whether lazy creation is the best solution.
+  optional<_VSTD::locale> __loc_;
+#endif
----------------
Lazy creation is fine but using `optional` is suboptimal since `locale` is basically a pointer.


================
Comment at: libcxx/include/format:18
+ * Partly due to the lack of recommended optimizations, partly since the
+ * committee is considering the following paper to be retroactively be applied
+ * to C++20.
----------------
It is no longer considering, the paper has been accepted.


================
Comment at: libcxx/include/format:241-245
+template <class _Context, class... _Args>
+_LIBCPP_INLINE_VISIBILITY __format_arg_store<_Context, _Args...>
+__make_format_args(const _Args&... __args) {
+  return {basic_format_arg<_Context>(__args)...};
+}
----------------
This is identical to `make_format_args` so it just adds unnecessary indirection and a ton of template instantiation. I suggest removing.


================
Comment at: libcxx/test/std/utilities/format/format.arguments/format.arg.store/class.pass.cpp:28
+
+template <class Context>
+void test() {
----------------
This better be parameterized on char type, not context (and the same throughout the tests).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103357