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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 3 inline comments as done.
Mordante added inline comments.


================
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 + "}"};
 
----------------
vitaut wrote:
> 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`?
Good suggestion! When I initially wrote this patch libc++ didn't support `constexpr std::string`, but now it does.


================
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];
----------------
vitaut wrote:
> 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.
It doesn't give a nice error. I've changed it to throwing an exception; still not a great error message but now the compiler output shows `"Argument index out of bounds"` instead of a null pointer error. I did the same for line 264.


================
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()});
+  }
----------------
vitaut wrote:
> 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.
I agree that this implementation may have more compile-time overhead than strictly needed. When I originally went an approach you suggested I was unhappy with the amount of code duplication. So for now I prefer to keep the current solution. I will probably look at it another time, then I have a baseline to measure the improvements against.


================
Comment at: libcxx/include/format:569-573
+#    ifdef _WIN32
+          __basic_format_string<char, type_identity_t<_Args>...> __fmt,
+#    else
+          __format_string<_Args...> __fmt,
+#    endif
----------------
vitaut wrote:
> Mordante wrote:
> > vitaut wrote:
> > > Can we avoid these ugly ifdefs? Why not rename `__format_string` to `__format_string_t` or similar?
> > I originally added them due to odd errors on Windows. Later I figured out it wasn't a bug of the Windows compilers but that `__format_string` is a SAL macro on Windows. (Not the first time I've been bitten by SAL macros.)
> > 
> > For now I prefer to keep them since I expect these become `format_string` during the plenary in July.
> Up to you but if you rename it to something other than `__format_string` now it will be a small change to switch to `format_string` as well.
Fair point, I've changed it.


================
Comment at: libcxx/include/format:236
+private:
+  void (*__parse_)(basic_format_parse_context<_CharT>&);
+};
----------------
philnik wrote:
> It's not necessary, but I find it a lot easier to read than finding the variable name inside a function pointer declaration.
As said above I prefer not to add a typedef for one instance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121530