[libcxx-commits] [PATCH] D103425: [libc++][format] Adds string formatter.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 9 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/CMakeLists.txt:19
+  __format/formatter_string.h
+  __format/parser_std_format_spec.h
   __function_like.h
----------------
vitaut wrote:
> Not sure if it's worth having such a fine-grained header split for built-in formatters considering that all of them are pretty small and end up being included in `<format>`.
Recently libc++ started to move towards using tiny headers. This started with the people working on ranges and I like that direction. So that's why I made these small headers. (This also makes it a lot easier to maintain larger patch series.)


================
Comment at: libcxx/include/__format/formatter.h:112-113
+ * @note The type of the input @a __first, @a __last can differ from the type
+ * of @a __fill. Integer output uses @c to_char for its conversion. This
+ * function returns a @c char type.
+ */
----------------
vitaut wrote:
> This function returns an iterator, not a char type.
This returns refers the the `to_char` function, which should have been `std::to_chars`. I added a `@returns` and improved this `@note` to avoid confusion.
This integer part isn't added yet, but is part of D103433.


================
Comment at: libcxx/include/__format/formatter.h:125
+  pair<size_t, size_t> __padding = __padding_size(__size, __width, __alignment);
+  __out_it = _VSTD::fill_n(_VSTD::move(__out_it), __padding.first, __fill);
+  __out_it = _VSTD::copy(__first, __last, _VSTD::move(__out_it));
----------------
vitaut wrote:
> Fill can consist of multiple code units.
For now libc++ uses one character as mentioned in D103368. I've added a comment as reminder that this needs to be adjusted in the future.


================
Comment at: libcxx/include/__format/formatter.h:138
+ * @param __precision The width to truncate the output to, use
+ *                    @ref __format::__number_max for no limit.
+ */
----------------
vitaut wrote:
> As commented elsewhere `__number_max` as currently defined is a valid precision and shouldn't be used for "no limit".
The new invalid value is `-1`.


================
Comment at: libcxx/include/__format/formatter_string.h:39
+template <class _CharT>
+class _LIBCPP_TEMPLATE_VIS __parser_string
+    : public __parser_std<_CharT, __parser_precision, __parser_no_sign,
----------------
vitaut wrote:
> This class looks unnecessary, why not fold it into `__formatter_string`?
In my original design it seemed nice, but now it's indeed unnecessary.


================
Comment at: libcxx/include/__format/formatter_string.h:87-88
+
+    if (this->__width > this->__precision) [[unlikely]]
+      __throw_format_error("Format-spec precision less then width");
+
----------------
vitaut wrote:
> Width being greater than precision is perfectly valid and useful. It shouldn't result in an error.
Interesting. I overlooked that in the Standard. I've adjusted to code to support this case and added a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103425