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

Mark de Wever via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Sun Jun 12 13:16:26 PDT 2022

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.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list