[libcxx-commits] [PATCH] D119567: [libcxx] String format class marked as packed

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:55
  */
-class _LIBCPP_TYPE_VIS _Flags {
+class _LIBCPP_TYPE_VIS __attribute__((__packed__)) _Flags {
 public:
----------------
Quuxplusone wrote:
> If I ran the zoo, I'd make this struct `__attribute__((__packed__))` //only on AIX//, and leave it "normal" everywhere else. I'm worried that a packed struct might behave observably differently on some non-AIX platform and have some effect that we didn't expect. But I'm not aware of any //concrete// problem with how you've got it now.
> 
> Pre-existing nit: I would also move lines 83–87 down next to line 110 to keep all the members together.
As commented below this `_Flags` class will be changed, so for now I don't mind the unconditional `__packed__`. I discovered some issues with the current design:
- A formatter is-a parser, when changing it to has-a parser it leads to a lot less code duplication in the binary. On an old branch I can reduce 12KB of object size. (I believe I can get an even better size reduction.)
- The is-a design makes it hard/impossible to make the format member function `const` qualified. This will become mandatory after LWG3636 you filed is accepted. I also believe this change makes it easier to implement P2286 Formatting ranges, which AFAIK is still targetting C++23.

In this new design I will test whether `__packed__` is still needed on AIX or not. (The goal is to keep the struct small so it can be passed in registers.) I learned godbolt has AIX support so that makes testing a lot easier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119567