[libc++][format] Implement format-string.

[Mordante]

Summary

Implements the compile-time checking of the formatting arguments.

Completes:

• P2216 std::format improvements

[Mordante]

libcxx/include/format

487

It seems __format_string is Microsoft annotation which caused the compilation failures. So by naming them different would solve the issue. This means the name becomes a bit awkward for the libc++ naming scheme. I expect P2508 to be accepted soon, which changes the names to format_string and wformat_string. For now I prefer to keep the work-around and fix it once P2508 is voted in. (This paper aims to be applied to C++20.)

[Mordante]

Rebased and resolved merge conflicts.

[h-vetinari]

Should probably be mentioned in the release notes...?

libcxx/docs/Status/Cxx20Papers.csv

199

There's several items on the Format status page that are (positively) affected by this as well.

[Mordante]

Addresses review comments.

[Mordante]

In D121530#3524487, @h-vetinari wrote:

Should probably be mentioned in the release notes...?

Good point, thanks!

libcxx/docs/Status/Cxx20Papers.csv

199

I tend to update page separately, that works better for me. Especially to mark things I started working on; several of these features are rather big.

[philnik]

libcxx/include/format

237

It's not necessary, but I find it a lot easier to read than finding the variable name inside a function pointer declaration.

[Mordante]

Rebased to fix CI.

[Mordante]

Fixes due to upstream changes.

[Mordante]

Try to fix CI.

[Mordante]

Rebased on main.

[vitaut]

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).

libcxx/benchmarks/formatter_float.bench.cpp

218–219

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?

libcxx/docs/ReleaseNotes.rst

124

typo: std:;format_to -> std::format_to

126

compile-time -> at compile time
compile time literal -> compile-time literal

128

run-time -> at run time.

libcxx/include/__format/parser_std_format_spec.h

260

Why make it public? Isn't it only called by subclasses?

libcxx/include/format

218

What is this class for? A comment would be nice.

260

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.

303

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.

314

Should be either "isn't integral" or "isn't an integral type"

474–475

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.

556–560

Can we avoid these ugly ifdefs? Why not rename __format_string to __format_string_t or similar?

[Mordante]

Thanks for the review! I've addressed some of the smaller comments, will look at the others tomorrow.

libcxx/include/__format/parser_std_format_spec.h

260

Not anymore, __compile_time_validate_argument calls the function too, there it needs to be public.

libcxx/include/format

218

I added comment. Basically when a handle is used the code needs to use the parse function of the formatter associated with the type _Tp.

303

Good point. Here it doesn't have an ABI impact. Based on earlier review comments by you in another patch I made these field unconditionally in the enum. So I can use them here unconditionally.

556–560

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.

[vitaut]

libcxx/include/format

556–560

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.

[Mordante]

Addresses review comments.

[Mordante]

libcxx/benchmarks/formatter_float.bench.cpp

218–219

Good suggestion! When I initially wrote this patch libc++ didn't support constexpr std::string, but now it does.

libcxx/include/format

237

As said above I prefer not to add a typedef for one instance.

260

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.

474–475

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.

556–560

Fair point, I've changed it.

[ldionne]

This LGTM. I think most comments by @vitaut have been applied, and while I have a couple of comments, I think this is good to go.

libcxx/docs/ReleaseNotes.rst

125

I think we should move these "API Changes" to "New features" above, but without mentioning that this can break code. Indeed, these release notes are aimed at LLVM releases, however <format> has not been enabled yet in LLVM releases, so calling this an API change is going to be misleading. I think we should only explain that format strings are now checked at compile-time.

libcxx/include/format

468–470

Would this work too? If so, it may even become unnecessary to have a __get_types function altogether, perhaps we can just inline it in the body of __basic_format_string below.

libcxx/test/std/utilities/format/format.functions/format.locale.pass.cpp

45

[Mordante]

Thanks for the review!

libcxx/include/format

468–470

Thanks for the suggestion, that removes quite some unneeded lines, especially for __get_handle.

[Mordante]

Addresses review comments.

[Mordante]

This should fix the AIX build.

[vitaut]

libcxx/benchmarks/formatter_float.bench.cpp

237

nit: no need to call make_fmt twice. In fact you could probably merge make_fmt into this lambda.

[Mordante]

Test to see whether the CI works again.

[Mordante]

libcxx/benchmarks/formatter_float.bench.cpp

237

I tried that, but that doesn't seem to work. The std::string isn't a constexpr then.

[vitaut]

libcxx/include/format

474–475

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?

[Mordante]

libcxx/include/format

474–475

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.

[Mordante]

libcxx/include/format

405

@vitaut The handle only parses the format string, it doesn't try to "format" it.

474–475

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.