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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125606