This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Finish P0645 Text Formatting.
ClosedPublic

Authored by Mordante on Dec 19 2021, 2:02 AM.

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Commits
rG4684857abfd7: [libc++][format] Finish P0645 Text Formatting.
Summary

This adjust the version macro and sets it as completed. All parts of the paper
have been implemented, except for the parts replaced by later papers and
LWG-issues.

Adjusted the synopsis to match the synopsis in the Standard. Not yet
implemented parts of P2216 and P2418 still use the P0645 wording.

Completes:

  • P0645 Text Formatting

Depends on D115991

Diff Detail

Event Timeline

Mordante created this revision.Dec 19 2021, 2:02 AM
Mordante requested review of this revision.Dec 19 2021, 2:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2021, 2:02 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

All parts of the paper have been implemented

Is the floating-point formatting fully implemented? It relies on to_chars or equivalent functionality to be available.

All parts of the paper have been implemented

Is the floating-point formatting fully implemented? It relies on to_chars or equivalent functionality to be available.

It's fully implemented in D114001 using to_chars. The to_chars implementation has been provided by Microsoft and has one minor caveat; it casts a long double to a double so it doesn't format correctly. (On Windows both types have the same underlying type, so it works fine for Microsoft.) But I consider that an issue in to_chars and not in <format>. Improving to_chars's long double support is on my todo list.

But I consider that an issue in to_chars and not in <format>.

Fair enough.

ldionne accepted this revision.Jan 24 2022, 7:39 AM
This revision is now accepted and ready to land.Jan 24 2022, 7:39 AM
Quuxplusone added inline comments.
libcxx/docs/ReleaseNotes.rst
43–45

Do we need to mention LIBCXX_ENABLE_INCOMPLETE_FEATURES here? I don't really know how it works.

libcxx/docs/Status/Cxx20.rst
44
ldionne requested changes to this revision.Jan 24 2022, 7:47 AM

Upon second thought, I think it would be best to leave the feature-test macro undefined until we actually guarantee ABI/API stability. Otherwise, users will just start using it and it will be difficult for us to say "well it was not stable, you should have waited". FTMs are basically the only way to communicate with our users in that respect, so I think we should be careful about not-quite-accurate information.

This revision now requires changes to proceed.Jan 24 2022, 7:47 AM
ldionne added inline comments.Jan 24 2022, 7:48 AM
libcxx/docs/ReleaseNotes.rst
43–45

My understanding is that since the original format paper will effectively fully implemented, we would not use LIBCXX_ENABLE_INCOMPLETE_FEATURES here. Was that your intent @Mordante?

Mordante added inline comments.Jan 24 2022, 8:48 AM
libcxx/docs/ReleaseNotes.rst
43–45

I intend to keep it. We added that in LLVM 13 since std::format wasn't ABI stable, which it's still not. There are some ABI breaking LWG issues and papers retroactively accepted in C++20. At the moment there's even one ABI breaking LWG issue active https://cplusplus.github.io/LWG/issue3576. Having it not ABI stable also makes it easier to look at possible optimizations in the implementation. I expect to be able to finish all remaining work in the LLVM 15 timeframe.

We could montion the macro, it's not really a new feature, but might be a nice reminder. (We use the same feature flag for our ranges implementation.)

Mordante updated this revision to Diff 402564.Jan 24 2022, 9:15 AM

Addresses review comments.

ldionne accepted this revision.Jan 24 2022, 9:25 AM
This revision is now accepted and ready to land.Jan 24 2022, 9:25 AM
This revision was landed with ongoing or failed builds.Jan 24 2022, 11:10 AM
This revision was automatically updated to reflect the committed changes.

Congratulations on landing this huge piece of work, and even more so in time for LLVM 14!

Congratulations on landing this huge piece of work, and even more so in time for LLVM 14!

Yes, absolutely, thanks a lot for your awesome work @Mordante. And thanks @vitaut for moving the reviews along.

Great work, @Mordante! Can't wait to use more of std::format on gobolt.

h-vetinari added a comment.EditedJan 24 2022, 6:17 PM

Just a nit/note before LLVM 14 branches: a few bullets in FormatPaper.csv should probably now be updated as well.

Confusingly, this CSV contains the actual chunks of the work, whereas FormatIssues.csv contains only the papers. Might be worth updating the names there, à la:

  • FormatPaper.csv -> FormatTasks.csv (or FormatSections.csv)
  • FormatIssues.csv -> FormatPapers.csv

Thanks all!

A big thanks to @vitaut for reviewing. Your insights have helped to improve the implementation!
A big thanks to @ldionne to make sure I didn't overload some libc++ coding issues.

Just a nit/note before LLVM 14 branches: a few bullets in FormatPaper.csv should probably now be updated as well.

Confusingly, this CSV contains the actual chunks of the work, whereas FormatIssues.csv contains only the papers. Might be worth updating the names there, à la:

  • FormatPaper.csv -> FormatTasks.csv (or FormatSections.csv)
  • FormatIssues.csv -> FormatPapers.csv

That was indeed on my todo list and I just updated them. I agree the naming of the files could be better. For now I keep them as is.

  • The number of open C++20 papers is quite limited.
  • For chrono and C++23 format/print related papers I hope/expect to use GitHub projects.