[libcxx-commits] [PATCH] D125606: [libc++][format] Improve string formatters

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 5 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:1459-1463
+  __alignment __alignment_ : 3;
+  __sign __sign_ : 2;
+  bool __alternate_form_ : 1;
+  bool __locale_specific_form_ : 1;
+  __type __type_ : 5;
----------------
ldionne wrote:
> Mordante wrote:
> > ldionne wrote:
> > > I think it's nice that this is really tailor-made to fit into as little space as possible, however I do have a concern that it might not be robust against future changes. For example, this code would break if we added a (admittedly large) number of elements to the `__type` enumeration. I'm not sure what would make me feel more easy about this -- perhaps simply adding some sort of `static_assert` to make sure the code stays in sync would be enough?
> > We currently have 18 members in the enum. I'm only aware of two papers intending to add a field
> > - http://wg21.link/P2510 "Formatting pointers" adds an upper case for pointer
> > - http://wg21.link/P2286 "Formatting Ranges" adds a debug output for chars and strings
> > 
> > This would leave use with 12 unused values. I can add one bit, which would give us 44 values to use.
> > 
> > I've added a `static_assert` for the entire struct.
> > 
> > 
> > 
> Do you know how other implementations handle this problem? What about @vitaut 's `fmt` library?
> 
> I'm sure there will be more stuff added to `<format>` in the future, but I have no idea of how much stuff is going to be added. Is it possible that we'll ever run into this limitation? I don't know. However, the situation I'd like to avoid is one where WG21 wants to move forward with a paper, but libc++ has to say "we won't be able to implement it due to ABI", while other implementations are able to implement it <for some reason>. If all other implementations make a tradeoff similar to this, I think it's fine.
> 
> Basically, I'd just like to make sure we're not painting our implementation into a corner.
fmt uses a struct with bitfields. It stores the type as an `enum class` with `unsigned char` as underlying value. This type isn't a bitfield.

MSVC STL uses a struct without bitfields and stores the type as the `char` used in the format string.

So if we want to avoid to be ever in a corner this field should not be a bit field. I feel it's a bit overkill, but it's not like the available space is tight, so I'll make this field 8 bits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125606