Page MenuHomePhabricator

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

Authored by Mordante on Mar 12 2022, 10:55 AM.

Details

Reviewers
EricWF
vitaut
ldionne
Group Reviewers
Restricted Project
Commits
rGaed5ddf8d097: [libc++][format] Implement format-string.
Summary

Implements the compile-time checking of the formatting arguments.

Completes:

  • P2216 std::format improvements

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Mordante added inline comments.May 14 2022, 12:35 PM
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 updated this revision to Diff 430462.May 18 2022, 11:29 AM

Rebased and resolved merge conflicts.

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 updated this revision to Diff 432710.May 28 2022, 4:02 AM

Addresses review comments.

Mordante marked an inline comment as done.May 28 2022, 4:06 AM

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 added inline comments.
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 updated this revision to Diff 432716.May 28 2022, 5:08 AM
Mordante marked an inline comment as done.

Rebased to fix CI.

Mordante updated this revision to Diff 432737.May 28 2022, 9:56 AM

Fixes due to upstream changes.

Mordante updated this revision to Diff 432750.May 28 2022, 1:08 PM

Try to fix CI.

Mordante updated this revision to Diff 432771.May 29 2022, 2:14 AM

Rebased on main.

vitaut requested changes to this revision.Jun 4 2022, 8:35 AM

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?

This revision now requires changes to proceed.Jun 4 2022, 8:35 AM
Mordante marked 8 inline comments as done.Jun 4 2022, 10:48 AM

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 added inline comments.Jun 4 2022, 11:38 AM
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 updated this revision to Diff 434315.Jun 5 2022, 3:31 AM
Mordante marked 6 inline comments as done.

Addresses review comments.

Mordante marked 3 inline comments as done.Jun 5 2022, 3:38 AM
Mordante added inline comments.
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 accepted this revision.Tue, Jun 7, 9:25 AM

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 marked 6 inline comments as done.Tue, Jun 7, 10:50 AM

Thanks for the review!

libcxx/include/format
468–470

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

Mordante updated this revision to Diff 434890.Tue, Jun 7, 10:53 AM
Mordante marked an inline comment as done.

Addresses review comments.

Mordante updated this revision to Diff 435050.Tue, Jun 7, 11:08 PM

This should fix the AIX build.

vitaut added inline comments.Wed, Jun 8, 9:11 AM
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 updated this revision to Diff 435289.Wed, Jun 8, 12:15 PM

Test to see whether the CI works again.

Mordante marked an inline comment as done.Wed, Jun 8, 12:20 PM
Mordante added inline comments.
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.

This revision was not accepted when it landed; it landed in state Needs Review.Sat, Jun 11, 6:26 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.
vitaut added inline comments.Sun, Jun 12, 9:26 AM
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 marked an inline comment as done.Sun, Jun 12, 1:16 PM
Mordante added inline comments.
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 marked an inline comment as done.Tue, Jun 14, 10:31 AM
Mordante added inline comments.
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.