[libcxx-commits] [PATCH] D121530: [libc++][format] Implement format-string.

[Victor Zverovich via Phabricator via libcxx-commits]

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

In general looks good but I would strongly recommend not to instantiate all of formatting path just to validate width/precision (see an inline comment).



================
Comment at: libcxx/benchmarks/formatter_float.bench.cpp:219-220
     std::array<char, 20'000> output;
     std::string fmt{std::string("{:") + Alignment<A::value>::fmt + Precision<P::value>::fmt +
                     Localization<L::value>::fmt + DisplayType<DT::value>::fmt + "}"};
 
----------------
Since all the components are known at compile time why not make format string construction `constexpr` and make the benchmark more realistic by replacing `vformat_to` with `format_to`?


================
Comment at: libcxx/docs/ReleaseNotes.rst:115
 
+- The format functions (``std::format``, ``std:;format_to``,
+  ``std::format_to_n``, and ``std::formatted_size``) now validate the format
----------------
typo: `std:;format_to` -> `std::format_to`


================
Comment at: libcxx/docs/ReleaseNotes.rst:117
+  ``std::format_to_n``, and ``std::formatted_size``) now validate the format
+  string compile-time. This means the argument needs to be a compile time
+  literal. This may break existing code. When the format string is invalid this
----------------
compile-time -> at compile time
compile time literal -> compile-time literal


================
Comment at: libcxx/docs/ReleaseNotes.rst:119
+  literal. This may break existing code. When the format string is invalid this
+  will make the code ill-formed instead of throwing an exception run-time.
+  (This does not affect the ``v`` functions.)
----------------
run-time -> at run time.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:260
    */
-  constexpr bool __has_width_field() const noexcept {
-    return __width_as_arg || __width;
-  }
+  constexpr bool __width_needs_substitution() const noexcept { return __width_as_arg; }
 
----------------
Why make it public? Isn't it only called by subclasses?


================
Comment at: libcxx/include/format:217
+template <class _CharT>
+class _LIBCPP_TEMPLATE_VIS __compile_time_handle {
+public:
----------------
What is this class for? A comment would be nice.


================
Comment at: libcxx/include/format:259
+  _LIBCPP_HIDE_FROM_ABI constexpr __arg_t arg(size_t __id) const {
+    // Accessing array out of bounds is UB thus an error.
+    return __args_[__id];
----------------
Does it give a user-friendly error? Perhaps it would be better to generate an error ourselves with a better message because this is a pretty common case.


================
Comment at: libcxx/include/format:302
+  case __arg_t::__long_long:
+#  ifndef _LIBCPP_HAS_NO_INT128
+  case __arg_t::__i128:
----------------
Is this ifdef needed? In principle you could always define `__i128` and `__u128` but only use it if `_LIBCPP_HAS_NO_INT128` is set. This might be beneficial for ABI compatibility and simplify some code like the one here.


================
Comment at: libcxx/include/format:313
+  default:
+    __throw_format_error("Argument isn't an integral");
+  }
----------------
Should be either "isn't integral" or "isn't an integral type"


================
Comment at: libcxx/include/format:502-503
+  consteval __basic_format_string(const _Tp& __str) : __str_{__str} {
+    __format::__vformat_to(basic_format_parse_context<_CharT>{__str_, sizeof...(_Args)},
+                           _Context{__types_.data(), __handles_.data()});
+  }
----------------
Using `__vformat_to` to do validation is an interesting idea but instantiation all of the formatting code just to check dynamic width/precision will likely have negative impact on compile times. I would recommend only doing parsing here and a small dynamic width/precision validation step. This should also make `__compile_time_basic_format_context` unnecessary.


================
Comment at: libcxx/include/format:569-573
+#    ifdef _WIN32
+          __basic_format_string<char, type_identity_t<_Args>...> __fmt,
+#    else
+          __format_string<_Args...> __fmt,
+#    endif
----------------
Can we avoid these ugly ifdefs? Why not rename `__format_string` to `__format_string_t` or similar?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121530