[libcxx-commits] [PATCH] D155366: [libc++][format] Improves run-time diagnostics.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM w/ comments.



================
Comment at: libcxx/include/__format/parser_std_format_spec.h:201
 enum class _LIBCPP_ENUM_VIS __type : uint8_t {
   __default,
   __string,
----------------



================
Comment at: libcxx/include/__format/parser_std_format_spec.h:223
 
+_LIBCPP_HIDE_FROM_ABI constexpr uint32_t __create_type_mask(__type __t) {
+  uint32_t __shift = static_cast<uint32_t>(__t);
----------------
`inline`.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:241-247
+inline constexpr uint32_t __type_mask_integer =
+    __create_type_mask<__type::__binary_lower_case,
+                       __type::__binary_upper_case,
+                       __type::__decimal,
+                       __type::__octal,
+                       __type::__hexadecimal_lower_case,
+                       __type::__hexadecimal_upper_case>();
----------------
WDYT about this? Then the `consteval` overload can be removed and IMO it's a bit more clear what's going on.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:442-443
+  //
+  // For some types all valid options need a second validation run, like
+  // a boolean types.
+  //
----------------



================
Comment at: libcxx/include/__format/parser_std_format_spec.h:445
+  //
+  // Depending on whether the validation is done compile-time or
+  // run-time the error differs
----------------



================
Comment at: libcxx/include/__format/parser_std_format_spec.h:447
+  // run-time the error differs
+  // - run-time the exception is thrown and contains the type of field
+  //   being validated.
----------------



================
Comment at: libcxx/include/__format/parser_std_format_spec.h:449
+  //   being validated.
+  // - compile-time the line with `std::__throw_format_error` is shown
+  //   in the output. In that case it's important the error is on one
----------------



================
Comment at: libcxx/include/__format/parser_std_format_spec.h:450-451
+  // - compile-time the line with `std::__throw_format_error` is shown
+  //   in the output. In that case it's important the error is on one
+  //   line.
+  // Note future versions of C++ may allow better compile-time error
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155366