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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I really like the new approach, especially since it cuts down on code size. I don't have any strong reservations about this patch, the only thing I'd really like is to investigate what the patch would look like if we did not jam the chrono and non-chrono formatter specs into the same structs. Other than that, I'm generally really happy with this -- thanks for making these improvements!



================
Comment at: libcxx/include/__format/parser_std_format_spec.h:1390-1391
+///
+/// They default to false so when a new field is added it needs to be opted in
+/// explicitly. That way the behaviour of user-defined formatters won't change.
+struct __fields {
----------------
I am not sure I understand this comment.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:1403-1407
+// By not placing this constant in the formatter class it's not duplicated for
+// char and wchar_t.
+inline constexpr __fields __fields_string{
+    .__precision_ = true,
+};
----------------
It seems to me like it might be cleaner to define it inside `formatter_string.h` (closer to its point of use), but this is just a suggestion, if you've been through these changes and feel like this is the right way to group things, go with it.


================
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;
----------------
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?


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:1465-1466
+
+  bool __weekday_name_ : 1;
+  bool __month_name_ : 1;
+
----------------
I find it a bit confusing that we're jamming these chrono specific fields here. I think we should either have a separate `__chrono_parsed_specifications` with just the fields it needs, or make this some sort of `union { <things-needed-by-std-format-spec>; <things-needed-by-chrono-format-spec> }`.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:1468
+
+  alignas(4) _CharT __fill_;
+
----------------
Do you think that makes sense? Otherwise, perhaps a comment is enough. Basically, it would be nice not to leave a raw hard-coded number unexplained, for newbies like me :-)


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:1553-1556
+  // LWG 3576 will probably change this to always accept a Unicode code point
+  // To avoid changing the size with that change align the field so when it
+  // becomes 32-bit its alignment will remain the same. That also means the
+  // size will remain the same. (D2572 addresses the solution for LWG 3576.)
----------------
Perhaps we could centralize this `4` value with this comment, and then reuse it here and above where I left a similar review comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125606