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

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut accepted this revision.
vitaut added a comment.

Looks great, just a few nits inline. Accepting the diff mostly as a reminder for myself that I've already looked at it.



================
Comment at: libcxx/include/__format/format_string.h:99-100
+__parse_number(const _CharT* __begin, const _CharT* __end_input) {
+  _LIBCPP_ASSERT(__format::__number_max == INT32_MAX,
+                 "The algorithm is implemented based on this value.");
+  /*
----------------
This could be a static assert.


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


================
Comment at: libcxx/include/format:608
+[[nodiscard]] _LIBCPP_INLINE_VISIBILITY const _CharT*
+__handle_replacement_string(const _CharT* __begin, const _CharT* __end,
+                            _ParseCtx& __parse_ctx, _Ctx& __ctx) {
----------------
nit: I'd call it `__handle_replacement_field` for consistency with the standard naming.


================
Comment at: libcxx/include/format:687
+        __throw_format_error(
+            "The format string contains an invalid escape sequence");
+
----------------
I would call it something like "unmatched '}'" rather than "an invalid escape sequence" because we don't know whether the intent was to have an escape sequence.


================
Comment at: libcxx/include/format:706
+  return __format::__vformat_to(
+      basic_format_parse_context{__fmt, __args.__size()},
+      basic_format_context{_VSTD::move(__out_it), __args});
----------------
I don't think you need to pass `__args.__size()` here because it is for compile-time checks only.


================
Comment at: libcxx/include/format:820
+  return __format::__vformat_to(
+      basic_format_parse_context{__fmt, __args.__size()},
+      basic_format_context{_VSTD::move(__out_it), __args, __loc});
----------------
same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664