[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 marked 11 inline comments as done and an inline comment as not done.
Mordante added a comment.

Thanks for the review!

In D103357#2819377 <https://reviews.llvm.org/D103357#2819377>, @vitaut wrote:

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

Good catch.



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


================
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:
----------------
vitaut wrote:
> Visiting an empty argument should pass `monostate` to the visitor, not throw an exception.
I thought this was an allowed optimization, but that seems not to be the case.
Adding `monostate` will require including `<variant>`. At the moment we're busy modularizing our headers, but that unveils some clang issues. So I'll include the entire header with a TODO.
This change will break some other patches; I'll update them later.


================
Comment at: libcxx/include/__format/format_arg.h:116
+  _LIBCPP_INLINE_VISIBILITY basic_format_arg() noexcept
+      : __bool{false}, __type_{__format::__arg_t::__none} {}
+
----------------
vitaut wrote:
> 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.
I recall I originally had it like that, but that caused some issues. However testing again it seems not to be required (anymore).


================
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)) {}
----------------
vitaut wrote:
> `basic_format_context` is not supposed to be constructed by users so these ctors better be private.
Not being able construct it makes it hard to test the implementation. So I prefer to keep them public.
MSVC STL also implements their constructors as public.


================
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
----------------
vitaut wrote:
> Lazy creation is fine but using `optional` is suboptimal since `locale` is basically a pointer.
True, want to look at a better solution later, hence the TODO. I initially wanted to focus on getting something working and optimize later.


================
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.
----------------
vitaut wrote:
> It is no longer considering, the paper has been accepted.
:-) (It was valid at the time of writing, I probably need to update the status of P2216 at more places.)


================
Comment at: libcxx/test/std/utilities/format/format.arguments/format.arg.store/class.pass.cpp:28
+
+template <class Context>
+void test() {
----------------
vitaut wrote:
> This better be parameterized on char type, not context (and the same throughout the tests).
Nice suggestion, that makes the callers a bit less verbose.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103357