[libcxx-commits] [PATCH] D96664: [libc++][format] Implement formatters.

Sun Jun 13 08:22:12 PDT 2021

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

Overall looks good, some comments inline.

================
Comment at: libcxx/include/__format/format_string.h:38
+template <class _CharT>
+[[nodiscard]] _LIBCPP_INLINE_VISIBILITY constexpr pair<const _CharT*, uint32_t>
+__parse_number(const _CharT* __begin, const _CharT* __end);
----------------
I would recommend using a struct with meaningful field names instead of a pair. In addition to making the code more readable it would remove dependency on utility.

================
Comment at: libcxx/include/__format/format_string.h:67
+    __throw_format_error(
+        "Format-spec numeric argument larger than the maximum");
+
----------------
Is it even possible to have 1 billion arguments to a function? I don't think so and therefore this should probably be an assert instead of an exception.

================
Comment at: libcxx/include/__format/format_string.h:91-93
+ * By limiting the maximum to 999.999.999 there's an easier way. Make sure the
+ * length of the input contains no more than 9 characters. In that case we only
+ * need to check against the end of the input.
----------------
The limit of `999.999.999` is OK for an argument ID but is unnecessarily restrictive for width and precision. {fmt} supports any nonnegative `int` value and so do common `printf` implementations (almost, some of them don't handle `INT_MAX`, but they do handle values >= 1,000,000,000: https://godbolt.org/z/Esb9e6Wxh).

Having such limit will make migration from `printf` and other formatting facilities more difficult, please remove or make sure that it's only used for argument IDs.

================
Comment at: libcxx/include/__format/format_string.h:122
+ *
+ * The parser will return a pointer beyond the last consumed argument. This
+ * should be the closing '}' of the arg-id.
----------------
I suggest changing "argument" to "character" to avoid confusion with formatting arguments and since individual characters are not really arguments, pointers to them are.

================
Comment at: libcxx/include/__format/format_string.h:140
+  if (*__begin < _CharT('0') || *__begin > _CharT('9'))
+    __throw_format_error("Format-spec arg-id starts with an invalid character");
+
----------------
nit: `format-spec` is a syntactic construct (http://eel.is/c++draft/format#string.general-1) so I suggest not capitalizing it or rephrasing the error message altogether (e.g. "Invalid character in arg-id" if it's common to have exception messages capitalized).

================
Comment at: libcxx/include/__format/formatter.h:34
+// Currently not implemented specializations throw an exception when used. This
+// is not conform the Standard. However not all Standard defined formatters
+// have been implemented yet. Until that time the current behavior is intended.
----------------
"is not conform" -> "does not conform to"

================
Comment at: libcxx/include/format:69
+  template<class... Args>
+    string format(string_view fmt, const Args&... args);
+  template<class... Args>
----------------
`string_view` should be replaced with `format_string<Args...>` per http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2216r3.html but I guess it can be done in a separate diff.

================
Comment at: libcxx/include/format:351
+template <class _Tp, class _CharT>
+struct _LIBCPP_TEMPLATE_VIS __formatter_char {
+  _LIBCPP_INLINE_VISIBILITY
----------------
Why add this instead of implementing parse/format in formatter specialization directly?

================
Comment at: libcxx/include/format:618
+    __throw_format_error("Format string terminated while parsing a replacement "
+                         "string or an escape seqence");
+
----------------
typo: sequence

================
Comment at: libcxx/include/format:632
+  case _CharT(':'):
+    // The arg-id has no format-specifier, advance the input to the format-spec.
+    __parse_ctx.advance_to(__next(__r.first, __end));
----------------
Looks like the meaning is negated here, this is exactly when we do have format-specifier.

================
Comment at: libcxx/include/format:645
+  _VSTD::visit_format_arg(
+      [&](auto&& __arg) {
+        formatter<decay_t<decltype(__arg)>, _CharT> __formatter;
----------------
Arguments should be passed by value since they are cheap to copy. Then decay_t will probably be unnecessary.

================
Comment at: libcxx/include/format:779
+            const _Args&... __args) {
+  // TODO FMT Improve PoC: using std::string is inefficient.
+  string __str = vformat(__fmt, _VSTD::make_format_args(__args...));
----------------
What does PoC stand for?

================
Comment at: libcxx/test/std/utilities/format/format.functions/tests.inc:113
+  {
+    // Note 128 bit support is only party implemented test the range
+    // conditions here.
----------------
party -> partly throughout the diff

================
Comment at: libcxx/test/std/utilities/format/format.functions/tests.inc:128-130
+  test(STR("hello 42.000000"), STR("hello {}"), static_cast<float>(42));
+  test(STR("hello 42.000000"), STR("hello {}"), static_cast<double>(42));
+  test(STR("hello 42.000000"), STR("hello {}"), static_cast<long double>(42));
----------------
You probably know that but the default format is wrong. The output should be `42` without decimal point or trailing zeros.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664