Skip to content

Conversation

cpplearner
Copy link
Contributor

This implements the uninteresting parts of P2321R2, namely changes to tuple, pair and vector<bool>::reference to make them usable as proxy references.

Partially addresses #2252.

@cpplearner cpplearner requested a review from a team as a code owner April 28, 2022 17:18
@strega-nil-ms strega-nil-ms self-assigned this Apr 28, 2022
@strega-nil-ms strega-nil-ms changed the title Implement proxy reference support in P2321R2 zip P2321R2 zip: Implement proxy reference support for zip in tuple, pair, and vector<bool>::reference Apr 28, 2022
@strega-nil-ms strega-nil-ms changed the title P2321R2 zip: Implement proxy reference support for zip in tuple, pair, and vector<bool>::reference P2321R2 zip: Make tuple, pair, and vector<bool>::reference indirectly_writable Apr 28, 2022
@StephanTLavavej StephanTLavavej added the cxx23 C++23 feature label Apr 29, 2022
@ghost

This comment was marked as outdated.

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.

Alright, I'm about halfway through the review; I'll comment and submit now and continue later.

  • tuple
  • type_traits
  • vector

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.

The rest looks reasonable to me; the only concern I have is using enable_if when requires would work.

@strega-nil-ms strega-nil-ms removed their assignment Apr 29, 2022
@StephanTLavavej
Copy link
Member

Because EDG doesn't fully support concepts yet, we use enable_if instead of requires, even in C++20/23 mode, when the code in question doesn't have an inherent dependency on concepts. Thus, it is proper to use enable_if within pair.

The eventual cleanup of this is tracked by #602.

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.

Given that we can't use requires here, this looks good to me; on a side note, it feels very weird to spend like 4 hours reviewing something and still not have any comments.

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.

This looks good to me; I had Billy look over my shoulder at it as well.

The only concern I had was adding a call to GetFileInformationByHandleEx, but was told it was inexpensive (and I noticed we're doing two calls anyways to get both basic and standard info), so I'm no longer concerned.

@StephanTLavavej
Copy link
Member

@strega-nil-ms I think your last comment was meant for #2373?

@StephanTLavavej
Copy link
Member

Thanks, this is great! 😻 I saw no issues with the extensive template machinery, so I went ahead and pushed changes for the minor issues and nitpicks I noticed (the concepts guard and the test.lst being notable although small). FYI @strega-nil-ms as you had previously approved.

@StephanTLavavej
Copy link
Member

⚠️ Note to self:

I need to export the namespace-scope machinery being added here.

@StephanTLavavej StephanTLavavej self-assigned this May 5, 2022
@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 f8748eb into microsoft:main May 5, 2022
@StephanTLavavej
Copy link
Member

Thanks for implementing this foundational machinery for zip! 🤐 😹 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants