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

[Louis Dionne via Phabricator via libcxx-commits]

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);
+static_assert(is_trivially_copyable_v<__parsed_specifications<char>>);
----------------
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.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125606