Skip to content

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

Merged
StephanTLavavej merged 6 commits into
microsoft:mainfrom
CaseyCarter:cifix
Aug 17, 2021
Merged

Avoid "friends of friends" (CWG1699) in common_iterator#2066
StephanTLavavej merged 6 commits into
microsoft:mainfrom
CaseyCarter:cifix

Conversation

@CaseyCarter
Copy link
Copy Markdown
Contributor

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
Comment thread stl/inc/iterator
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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

miscco commented Jul 20, 2021

LGTM except that I am missing some test coverage

@CaseyCarter
Copy link
Copy Markdown
Contributor 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
Copy Markdown
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.

Comment thread stl/inc/iterator Outdated
Comment thread tests/std/tests/P0896R4_common_iterator/test.cpp Outdated
Comment thread tests/std/tests/P0896R4_common_iterator/test.cpp Outdated
@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
Copy Markdown
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
Copy Markdown
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