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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__format/format_arg.h:144
+
+  union {
+    bool __bool;
----------------
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.


================
Comment at: libcxx/include/__format/format_args.h:62
+  size_t __size_{0};
+  const basic_format_arg<_Context>* __data_{nullptr};
+};
----------------
This is the array-of-structs that @vitaut pointed to. It should be rewritten as a struct-of-arrays.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103357