Skip to content

Conversation

@tylerbrawl
Copy link
Contributor

@tylerbrawl tylerbrawl commented Jan 3, 2023

As a follow up to #3035, this PR implements std::views::zip_transform from P2321R2 as described in the C++ Working Draft. The following defect reports (DRs) and revisions from #2252 have also been implemented:

One thing to note about the current implementation is that in [range.zip.transform.overview]/2.1.2 of the C++ Working Draft, it is written that ((void)F, auto(views::empty<decay_t<invoke_result_t<FD&>>>)) is a potential equivalent expression when using the std::views::zip_transform customization point object. This makes use of the decay-copy language feature proposed in P0849R8, but since that hasn't been implemented yet in the MSVC, the current implementation instead gets the relevant type manually. I would like further input on approaching this problem (use the current solution, wait for compiler support, use the older decay-copy() function, etc.).

Lastly, this will be marked as a draft release until test cases are implemented. As always, let me know if you have any questions.

Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some thoughts on empty_view and iterator_category...

@CaseyCarter CaseyCarter added the cxx23 C++23 feature label Jan 4, 2023
@tylerbrawl tylerbrawl marked this pull request as ready for review January 5, 2023 15:39
@tylerbrawl tylerbrawl requested a review from a team as a code owner January 5, 2023 15:39
@tylerbrawl

This comment was marked as resolved.

Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes are needed to conform to [range.zip.transform.overview]/2.

@StephanTLavavej StephanTLavavej added the ranges C++20/23 ranges label Jan 11, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jan 11, 2023
@StephanTLavavej StephanTLavavej removed the decision needed We need to choose something before working on this label Jan 25, 2023
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes requested

@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej
Copy link
Member

Pushed changes to address @strega-nil-ms's comments, plus minor things I noticed along the way:

  • Rename to _Subscript_closure.
  • Rename to _Get_iterator_inner.
    • Also drop a couple of unnecessary explicit template arguments.
  • Shorten _Iter_sent_difference_type (to avoid wrapping weirdly).
  • Remove clang-format suppressions.
  • Use C-style comments for /* strengthened */ to avoid wrapping braces on the next line.
  • Change _Is_end_noexcept to be a _CONSTEVAL function.

@StephanTLavavej StephanTLavavej self-assigned this Jan 27, 2023
@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 work around an internal compiler assertion, VSO-1732554 "x86chk assertion failure in immediately invoked lambda computing iterator_category: RPF_ISSET(TypeSeenThisDepth) || Parser::Utility::CurrentlyInRdParser(), p1inl.c 5344". The workaround is to use a chain of conditional_ts.

@StephanTLavavej
Copy link
Member

I had to push another commit to work around #1030. The internal build uses native compilers for x86 (eww 🙀 🤮) and /analyze's higher memory consumption exceeds 4 GB for at least the random-access configuration (and is dangerously high for the others).

The really expensive thing is the matrix of 8 permutations. Instead of skipping everything for /analyze, I chose to skip 7 out of 8, to leave at least some coverage. (I'm not too worried about /analyze-specific warnings or problems, given this residual coverage.)

@CaseyCarter
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephanTLavavej StephanTLavavej merged commit f8b8ad6 into microsoft:main Jan 28, 2023
@StephanTLavavej
Copy link
Member

Thanks for implementing another major view in this paper! 🤐 🔀 😻

@JMazurkiewicz

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx23 C++23 feature ranges C++20/23 ranges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants