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

Avoid "friends of friends" (CWG1699) in common_iterator #2066

Merged
merged 6 commits into from
Aug 17, 2021

Conversation

CaseyCarter
Copy link
Member

CWG consensus is that friend functions defined inline in a class X should be treated as friends of classes that befriend X, but there's implementation divergence. (Probably because the issue has been in "drafting" for eight years.)

Fixes #2065

CWG consensus is that friend functions defined inline in a class `X` should be treated as friends of classes that befriend `X`, but there's implementation divergence. (Probably because the issue has been in "drafting" for eight years.)

Fixes microsoft#2065
@CaseyCarter CaseyCarter added the bug Something isn't working label Jul 20, 2021
@CaseyCarter CaseyCarter requested a review from a team as a code owner July 20, 2021 18:55
template <input_or_output_iterator _OIter, sentinel_for<_OIter> _OSe>
requires (!same_as<_OIter, _OSe> && copyable<_OIter>)
friend class common_iterator;
// clang-format on
Copy link
Member Author

@CaseyCarter CaseyCarter Jul 20, 2021

Choose a reason for hiding this comment

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

Note the removal of cross-specialization friendship here; I've chosen to use _Get_val consistently in both member functions and friend functions.

@miscco
Copy link
Contributor

miscco commented Jul 20, 2021

LGTM except that I am missing some test coverage

@CaseyCarter
Copy link
Member Author

LGTM except that I am missing some test coverage

Something funny is going on: we are somehow missing cross-type comparison coverage, but we do have cross-type difference coverage which is not failing.

@StephanTLavavej StephanTLavavej self-assigned this Jul 21, 2021
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good, I'll validate and push simple changes.

stl/inc/iterator Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_common_iterator/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_common_iterator/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej

This comment has been minimized.

@mnatsuhara mnatsuhara removed their assignment Aug 13, 2021
@StephanTLavavej StephanTLavavej self-assigned this Aug 14, 2021
@StephanTLavavej
Copy link
Member

I'm mirroring this to an MSVC-internal PR. Please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 9288cf1 into microsoft:main Aug 17, 2021
@StephanTLavavej
Copy link
Member

Thanks for fixing this friendship bug! 😸 🎉 🚀

@CaseyCarter CaseyCarter deleted the cifix branch August 21, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<iterator>: The implementation of common_iterator::operator== seems to be wrong
4 participants
  翻译: