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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added inline comments.


================
Comment at: libcxx/include/format:405
+          else if constexpr (same_as<decltype(__arg), typename basic_format_arg<_Ctx>::handle>)
+            __arg.format(__parse_ctx, __ctx);
+          else {
----------------
@vitaut The handle only parses the format string, it doesn't try to "format" it.


================
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()});
+  }
----------------
Mordante wrote:
> vitaut wrote:
> > Mordante wrote:
> > > 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.
> > You can avoid duplication with some refactoring of parsing code but I guess it doesn't have to be done in the current diff.
> > 
> > But does the current approach work with user-defined formatters that don't have templated `format` methods and therefore won't accept `__compile_time_basic_format_context`? Is it covered in tests?
> Good question. The current handle test is templated (using `auto`) I'll change that formatter to match the BasicFormatter requirements. I'll make a separate review for that.
I've validated it.
- I always use a `basic_format_parse_context<CharT>` as parse context must work due to the `BasicFormatter` requirements.
- I only instantiate a `_compile_time_basic_format_context` if the type isn't a handle class. See my new comment.

So this is work, but there's no test. I've updated the handle test in D127767.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121530