Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add range_formatter #4642

Merged
merged 37 commits into from
May 30, 2024
Merged

Conversation

JMazurkiewicz
Copy link
Contributor

  • Remove used by pair, tuple, (...) comment from _Fill_align_and_width_specs - it no longer serves its purpose.
  • Make _Fill_align_and_width_specs_setter<T>::_Specs member variable protected, so that we can create derived _Range_specs_setter.
  • Implement range_formatter:
    • range_formatter::format can be used at runtime only if format context's output iterator is back_insert_iterator<_Fmt_buffer<T>>. Creation of nested basic_format_context requires copying format args from old context<X> to context<Y> and X and Y have to be the same type to do this. This should not be a problem anyway, since basic_format_context can be constructed only by the standard library.
    • Towards P2286R8 Formatting Ranges #2919.

@JMazurkiewicz JMazurkiewicz requested a review from a team as a code owner April 29, 2024 19:58
stl/inc/format Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added ranges C++20/23 ranges format C++20/23 format cxx23 C++23 feature labels Apr 30, 2024
@StephanTLavavej StephanTLavavej self-assigned this Apr 30, 2024
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks, this is super awesome! 😻 I pushed a bunch of nitpicky changes, plus one bugfix and test coverage for it.

@StephanTLavavej StephanTLavavej removed their assignment May 29, 2024
@StephanTLavavej StephanTLavavej self-assigned this May 29, 2024
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej
Copy link
Member

I had to push an additional commit to dramatically reduce compiler memory consumption (and improve test throughput) by reducing the number of input ranges being tested.

Memory consumption was a severe problem for the MSVC-internal mirror for a couple of reasons:

  • MSVC-internal uses an x86-native compiler, unlike the GitHub PR/CI system which uses x64-hosted x86-targeting,
  • MSVC-internal uses a "checked" compiler with assertions enabled, which consumes slightly more memory.

Together, this imposed a 4 GB limit, and /analyze configurations (which are extremely memory-hungry) ran out of memory.

With my changes, MSVC-internal compiler memory consumption (as measured by /d1reportMemorySummary; that switch works for the public build too) decreased from 2280 MB to 395 MB for plain, and from 4GB+ to 718 MB for /analyze.

This also dramatically improved test throughput. On the GitHub side, with my physical machine and the x64-native compiler, test execution time dropped from 114 seconds to 18 seconds, a delicious 6.3x speedup.

@StephanTLavavej StephanTLavavej merged commit 0d32e40 into microsoft:main May 30, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for this significant step in one of the last remaining C++23 features! 😻 📈 ⭐

@JMazurkiewicz JMazurkiewicz deleted the format/ranges branch May 30, 2024 18:09
@JMazurkiewicz JMazurkiewicz restored the format/ranges branch June 4, 2024 19:43
@JMazurkiewicz JMazurkiewicz deleted the format/ranges branch June 4, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature format C++20/23 format ranges C++20/23 ranges
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants