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

Why shouldn't we replace _Ref_fn with reference_wrapper? #4027

Closed
frederick-vs-ja opened this issue Sep 14, 2023 · 3 comments · Fixed by #4036
Closed

Why shouldn't we replace _Ref_fn with reference_wrapper? #4027

frederick-vs-ja opened this issue Sep 14, 2023 · 3 comments · Fixed by #4036
Labels
documentation Related to documentation or comments fixed Something works now, yay!

Comments

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Sep 14, 2023

In #4025 there's attemption to strengthen the exception specification of _Ref_fn::operator().

Given the standard class template std::reference_wrapper is already a invocable wrapper and its operator() has a propagating exception specification (thanks to LWG-3764), it seems OK to replace the uses of _Ref_fn with reference_wrapper.

Pros:

  • user codes should only use reference_wrapper, so we might be able to instantiate less specializations.

Cons:

  • reference_wrapper is slightly heavier than _Ref_fn;
  • reference_wrapper is assignable, which might be unwanted for the internal details of the implementation.
@frederick-vs-ja frederick-vs-ja added the question Further information is requested label Sep 14, 2023
@CaseyCarter
Copy link
Contributor

IMO we should not replace _Ref_fn with reference_wrapper, because reference_wrapper is substantially heavier. _Ref_fn is an aggregate so it can be enregistered; reference_wrapper is not. This was the original motivation for creating _Ref_fn; "we can't accidentally assign to it" is nice, but easily achievable with const reference_wrapper.

@frederick-vs-ja
Copy link
Contributor Author

@CaseyCarter Thanks for explonation! Is this design choice documented anywhere?

@CaseyCarter
Copy link
Contributor

Sadly no. Our implementation documentation practices were nearly non-existent before open sourcing. The team was so small (1-3 maintainers) that everyone either wrote or reviewed every change, so we didn't really need documentation. Let's turn this into a documentation issue, and I'll submit a quick comment change to fix it.

@CaseyCarter CaseyCarter added documentation Related to documentation or comments and removed question Further information is requested labels Sep 18, 2023
@CaseyCarter CaseyCarter changed the title Should we replace _Ref_fn with reference_wrapper? Wy shouldn't we replace _Ref_fn with reference_wrapper? Sep 18, 2023
@CaseyCarter CaseyCarter changed the title Wy shouldn't we replace _Ref_fn with reference_wrapper? Why shouldn't we replace _Ref_fn with reference_wrapper? Sep 18, 2023
CaseyCarter added a commit to CaseyCarter/STL that referenced this issue Sep 18, 2023
@StephanTLavavej StephanTLavavej added the fixed Something works now, yay! label Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Related to documentation or comments fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants