[libcxx-commits] [PATCH] D103368: [libc++][format] Adds parser std-format-spec.

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added inline comments.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:421
+    _LIBCPP_ASSERT(__precision_as_arg == 1,
+                   "Substitute precision called without when no substitution "
+                   "is required.");
----------------
Looks like a stray "without".


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:471
+
+/** Parses the type and validates the presence of the final '}'. */
+class _LIBCPP_TYPE_VIS __parser_type {
----------------
This is a wrong place to do such validation because format specs may not have `}`.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:551
+/**
+ * The parser for the std-format-space.
+ *
----------------
Did you mean `std-format-spec`?


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:556-558
+ * @note The standard doesn't mandate parsing to be constexpr this is an
+ * extension. https://wg21.link/P2216 will mandate the parsing happens at
+ * compile time.
----------------
It does now.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:582
+   * directly after the ':'.
+   * @pre The std-format-spec is terminated by a '}'. The standard allows to
+   * terminate at __parse_ctx.end(), but the replacement string shall end with
----------------
This isn't correct. `std-format-spec` doesn't have to be terminated by anything. `formatter::parse` can be used to parse standalone format specs. `}` is part of `replacement-field` and should be handled there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103368