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

<format>: delay vformat instantiation #2331

Merged
merged 5 commits into from
Nov 13, 2021
Merged

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Nov 10, 2021

Fixes #2329

@fsb4000 fsb4000 requested a review from a team as a code owner November 10, 2021 15:33
@fsb4000

This comment has been minimized.

@CaseyCarter CaseyCarter added the throughput Must compile faster label Nov 10, 2021
@CaseyCarter CaseyCarter added the format C++20/23 format label Nov 10, 2021
@CaseyCarter

This comment has been minimized.

@StephanTLavavej StephanTLavavej self-assigned this Nov 10, 2021
@StephanTLavavej

This comment has been minimized.

@StephanTLavavej
Copy link
Member

  • Reported VSO-1433873 "Standard Library Header Units: Adding template <int = 0> to vformat() emits warnings C4265 and C4365".
    • Reported internally instead of on DevCom because my repro was partially reduced to "check out and build this branch in my fork of the STL" (which @cdacamar has expressed a preference for, but which DevCom triage presumably finds problematic).
  • Adjusted preprocessor logic to #if defined(__clang__) || defined(__EDG__), which distinguishes Clang/EDG from C1XX.
    • In general, _MSC_VER is always defined (everyone imitates C1XX), so that doesn't indicate a particular front-end.
  • Also adjusted the comments to our conventional // ^^^ no workaround / workaround vvv form.
  • Extracted the workaround into a macro _TEMPLATE_INT_0_NODISCARD (later #undef'ed) because it needs to be repeated 4x, and clang-format otherwise produces ugly results.
  • Added a comment improves throughput, see GH-2329.
    • In general, doing anything subtle that isn't required by the Standard is typically a good reason to have a comment. Mentioned briefly in the issue: "We should probably have comments as to why we're doing this."
  • The workaround is repeated for all 4 overloads because the (string_view, format_args) overload isn't special - it was just the only one being tested in P1502R1_standard_library_header_units. Adding a wide string test case revealed that the compiler issue also affected (wstring_view, wformat_args). So, I'm permanently adding wide coverage. (And slightly enhancing the test case to stringify an int 1729, although that didn't reveal anything.) I didn't think that adding locale cases was necessary, but the workaround is almost certainly needed for them too, so I did that.
  • I am not sure if the warning C4265 (non-virtual dtor) is valid - _Fmt_iterator_buffer certainly has virtual functions with a non-virtual dtor, but we never delete it through a pointer-to-base (it's always a local variable on the stack). However, the warning C4365 occurrences were actually valid - <format> was passing the result of pointer subtraction, which is ptrdiff_t, to _Count_separators() which takes size_t, so I'm adding static_casts:
    _NODISCARD inline int _Count_separators(size_t _Digits, const string_view _Groups) {

@StephanTLavavej StephanTLavavej removed their assignment Nov 11, 2021
@StephanTLavavej StephanTLavavej self-assigned this Nov 12, 2021
@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 4a2424c into microsoft:main Nov 13, 2021
@StephanTLavavej
Copy link
Member

Thanks for improving <format> throughput and finding the modules bug here (which has been fixed)! 🎉 📈 😻

@fsb4000 fsb4000 deleted the fix2329 branch November 13, 2021 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format C++20/23 format throughput Must compile faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<format>: inline overloads of vformat() are costly for throughput
3 participants