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

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut requested changes to this revision.
vitaut added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/CMakeLists.txt:19
+  __format/formatter_string.h
+  __format/parser_std_format_spec.h
   __function_like.h
----------------
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>`.


================
Comment at: libcxx/include/__format/formatter.h:60
+
+[[nodiscard]] _LIBCPP_INLINE_VISIBILITY inline constexpr pair<size_t, size_t>
+__padding_size(size_t __size, size_t __width,
----------------
As in earlier diffs I suggest using a struct instead of pair.

Also `inline` seems redundant.


================
Comment at: libcxx/include/__format/formatter.h:98
+ * @pre [@a __first, @a __last) is a valid range.
+ * @pre @a __size < = @a __width. Using this function when this pre-condition
+ *      doesn't hold incurs an unwanted overhead.
----------------
Is the space between `<` and `=` intentional?


================
Comment at: libcxx/include/__format/formatter.h:106
+ *                    to be written are ASCII the following condtion holds
+ *                    @a __size == @a __last - @a __first. For Unicode this
+ * @param __width     The number of output columns to write.
----------------
Something is missing after "For Unicode this".


================
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.
+ */
----------------
This function returns an iterator, not a char type.


================
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));
----------------
Fill can consist of multiple code units.


================
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.
+ */
----------------
As commented elsewhere `__number_max` as currently defined is a valid precision and shouldn't be used for "no limit".


================
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,
----------------
This class looks unnecessary, why not fold it into `__formatter_string`?


================
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");
+
----------------
Width being greater than precision is perfectly valid and useful. It shouldn't result in an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103425