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

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added inline comments.


================
Comment at: libcxx/include/__format/format_arg.h:144
+
+  union {
+    bool __bool;
----------------
Quuxplusone wrote:
> vitaut wrote:
> > ldionne wrote:
> > > Why aren't we using a `variant` here?
> > > 
> > > @vitaut I'm being told you originally requested that Mark moves from `variant` to this `union`, was there a reason? I'd like to avoid going back and forth on this issue.
> > Not using `variant` allows optimizing the representation for the common case of a small number of formatting arguments: http://eel.is/c++draft/format#args-note-1
> > 
> > > Implementations should optimize the representation of basic_­format_­args for a small number of formatting arguments.
> > > [Note 1: For example, by storing indices of type alternatives separately from values and packing the former. — end note]
> This open-coded `__arg_t __type_` member takes only 1 byte; libc++'s <variant> would use a field `__variant_index_t __index_` which takes 4 bytes //unless// `_LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION` is set.
> See D40210.
> The Right Answer here might depend on how close we think we are to "ABI v2 by default". Currently that looks like "never". ;)
> 
> However however, my understanding of @vitaut's comment is that this code still is not implementing the recommended practice. The recommended practice is that `basic_format_args` shouldn't store an array of `basic_format_arg {union, char}` structs at all; it should store a struct of arrays (one array of unions and one array of chars).
> 
> I don't think there's any motivation to optimize the layout of `basic_format_arg` singular. Using `variant` would be fine, except that it would introduce an extra dependency on `<variant>` and probably harm compile times.
My understanding is that the optimization is planned for a follow-up diff in which case switch to `variant` would have to be undone. cc @Mordante 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103357