[libcxx-commits] [PATCH] D103357: [libc++][format] Add __format_arg_store.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added a comment.

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

> LGTM but this will need to be reviewed by someone familiar with libc++ of course =).

Thanks for the review. Yeah the final approval will be done by someone more familiar with libc++.



================
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:
> Mordante wrote:
> > 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.
> Microsoft STL pulled out monostate into a separate header to avoid including variant.
I plan to do the same. I added a todo at line 20.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103357