[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: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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121530