[libcxx-commits] [PATCH] D96664: [libc++][format] Implement formatters.
Victor Zverovich via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
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
More information about the libcxx-commits
mailing list