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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Jun 20 11:39:38 PDT 2022

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.

Comment at: libcxx/include/__format/parser_std_format_spec.h:1468
+/// This struct doesn't cross ABI bounds so it doesn't need to be ABI stable.
+template <class _CharT>

Comment at: libcxx/include/__format/parser_std_format_spec.h:1470
+template <class _CharT>
+struct _LIBCPP_TEMPLATE_VIS __parsed_specifications {
+  union {
We don't need `visibility(default)`. First of all, it doesn't change anything because all the member functions are marked with `HIDE_FROM_ABI` (which gives them `visibility(hidden)`) and there's no vtable to give visibility to, and secondly it's kind of confusing to use a visibility annotation other than `_LIBCPP_HIDDEN` on a struct that we call out as being not part of the ABI.

Comment at: libcxx/include/__format/parser_std_format_spec.h:1477
+    // This is needed since the generic output routines handle the alignment of
+    // the output and therefore need this field.
+    __alignment __alignment_ : 3;

Comment at: libcxx/include/__format/parser_std_format_spec.h:1502
+static_assert(sizeof(__parsed_specifications<char>) == 16);
Can you add a comment explaining *why* you perform those tests (something like `// make sure it fits in a register and it's trivially copyable for performance`)? Otherwise, it looks as-if you are trying to enforce some ABI stability, which would be counter to the comment above (and your actual intent IIUC).

Comment at: libcxx/include/__format/parser_std_format_spec.h:1515
+/// must be ABI stable. To aid the stability the unused bits in the class are
+/// set to zero. That way they can be repurposed when a future revision of the
+/// Standards adds new fields to std-format-spec.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list