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

My Little Operator: Friendship Is Magic #2797

Merged
merged 11 commits into from
Jun 20, 2022

Conversation

StephanTLavavej
Copy link
Member

This changes all of the STL's operators that are required to exist, but that aren't required to be namespace-scope functions, into "hidden friends". (For example, n + vector::iterator.)

Hidden friends are great in two different ways:

  • They improve compiler throughput, because they don't pollute unqualified name lookup. Only argument-dependent lookup finds them when they're needed.
  • They automatically work with named modules, when their parent class (or a function returning their parent class, etc.) is exported. (Namespace-scope operators would have to be individually exported.)

A few notes:

  • span::iterator has used hidden friends since <span>: fix cross-type iterator operations #474 and currently says:
    _NODISCARD_FRIEND constexpr _Span_iterator operator+(const difference_type _Off, _Span_iterator _Next) noexcept {
  • The <iomanip> diff looks horrible, but I've structured it into a series of commits for easier reviewing.
    • The _Elem and _Elem2 template parameters have to "trade places" when becoming hidden friends. We static_assert that they're the same, so there is no potential for mistakes.
  • quoted() needed extra attention due to how its operators referred to each other. I recommend diffing the "before" and "after" parts of each commit.
    • First, I made this easier to deal with by moving _Quote_out before _Quote_in.
    • In "Use friendship for _Quote_out.", I had to rename to _OsTraits to avoid a collision, and use class _QuTraits = _Traits both for clarity and to delay evaluation of is_void_v<_QuTraits>.
    • In "Use friendship for _Quote_in, part 1.", instead of calling quoted(), I directly construct a _Quote_out (thanks to the earlier code movement).
    • Finally, "Use friendship for _Quote_in, part 2." is nearly pure code movement.

🐴 🪄 🦄

@StephanTLavavej StephanTLavavej added the throughput Must compile faster label Jun 16, 2022
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner June 16, 2022 10:34
Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

That is soo much better

@StephanTLavavej StephanTLavavej self-assigned this Jun 19, 2022
@StephanTLavavej
Copy link
Member Author

I'm speculatively mirroring this to the MSVC-internal repo. Further changes can be pushed, but please notify me.

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Ugh, <iomanip> was a mess to review.

@StephanTLavavej
Copy link
Member Author

Ugh, <iomanip> was a mess to review.

Thanks for slogging through it - import-std will be much easier to review thanks to this 😻

@DanielaE
Copy link

Stephan,
regarding the second bullet point in your opening post please be aware of CWG2588. We've been discussing this in EWG just last week. The one proposed solution which got consensus is the only reasonable one IMHO. So you're probably safe but I'm not sure if the compiler already implements these semantics yet.

@StephanTLavavej
Copy link
Member Author

@DanielaE Thanks for the heads up! 😸

@StephanTLavavej StephanTLavavej merged commit ee43e70 into microsoft:main Jun 20, 2022
@StephanTLavavej StephanTLavavej deleted the friendly-operators branch June 20, 2022 01:01
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
throughput Must compile faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants