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

Implement formatter<vector<bool>::reference> #4133

Merged

Conversation

frederick-vs-ja
Copy link
Contributor

Towards #2919.

This PR contains 3 parts:

  1. Addressing review comments from @CaseyCarter in the previous PR Implement formattable, range_format, and format_kind #4116.
  2. Splitting a small part of <format> needed for the definition of std::formatter (but not its critical member functions) to a new internal header <__msvc_formatter.hpp>.
    1. I believe <vector>, which is one of the most frequently used headers of STL, shouldn't include the whole <format>.
    2. Per [formatter.requirements], IIUC the parse and format functions are only required to be usable if basic_format_parse_context and basic_format_context are available respectively, which is only guaranteed by including <format>. So, I think it's safe to separate their definitions into <format>.
    3. The inclusion in <thread> is reduced as only _UIntegral_to_buff is necessary.
    4. <stacktrace> is currently unchanged because it contains non-template parse member function of a full specialization. Perhaps we can optimize it later.
  3. Implementation of formatter<vector<bool, Alloc>::reference> and test coverage for it.

MSVC-internal changes are needed for the new header <__msvc_formatter.hpp>.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner October 28, 2023 17:50
@StephanTLavavej StephanTLavavej added ranges C++20/23 ranges format C++20/23 format cxx23 C++23 feature labels Oct 30, 2023
stl/inc/vector Outdated Show resolved Hide resolved
stl/inc/__msvc_formatter.hpp Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/__msvc_formatter.hpp Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter self-assigned this Nov 1, 2023
@StephanTLavavej

This comment was marked as outdated.

@StephanTLavavej StephanTLavavej removed their assignment Nov 16, 2023
@StephanTLavavej

This comment was marked as resolved.

stl/inc/format Show resolved Hide resolved
stl/inc/format Show resolved Hide resolved
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej
Copy link
Member

I pushed a conflict-free merge with main followed by:

  • Use TYPED_LITERAL as @JMazurkiewicz suggested.
  • Pre-existing: _Fill_align_and_width_formatter::parse => _Parse. This member function isn't exposed to users via inheritance, so it should be ugly, like _Format already is.
  • Pre-existing: <stacktrace> should guard #include <format> with __cpp_lib_concepts.
    • Its usage below is already guarded.
  • Undo attempted throughput improvement by moving _Fill_align_and_width_MEOW back to <format>.
    • I believe we could get this to work by using the same technique as the following commit, but this PR is already doing a lot - we shouldn't add major drive-by throughput improvements. (The <string> inclusion improvement is already enough.)
  • Work around VSO-1236041 (header units bug).
    • The compiler bug specifically affects member functions (including but not limited to constructors) being declared in one header and defined in another. The STL has historically avoided that, which is why it doesn't usually impact us. My workaround is to declare non-member functions to do the work, which is fairly easy because we only have to pass one data member.

@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 8234695 into microsoft:main Nov 29, 2023
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks for formatting part of everyone's favorite data structure! 😹 👻 🎉

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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants