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

Add support for registering event handlers with shared_ptr and weak_ptr #1330

Merged

Conversation

sylveon
Copy link
Contributor

@sylveon sylveon commented Jul 9, 2023

Fixes #1316

@kennykerr
Copy link
Collaborator

The implementation seems reasonable, but I'll let @dmachaj sign off.

@dmachaj
Copy link
Contributor

dmachaj commented Jul 10, 2023

Do either of you know if these delegates are pruned by the linker if there is no usage? The only meta-question I have is regarding binary size. If these delegates have two additional vtable slots each, and they are not pruned by the linker when unused, then that might add up across a large code base.

@dmachaj
Copy link
Contributor

dmachaj commented Jul 10, 2023

@sylveon thank you for taking this on 🙂. I am now back from my vacation and it was a nice thing to see this PR at the top of my inbox. I think your changes here (especially the test coverage) are better than anything I would have been able to write 👍.

@sylveon
Copy link
Contributor Author

sylveon commented Jul 10, 2023

Since all the implementation is templated constructors, I expect them to never even show up to the linker until used (because the compiler wouldn't instantiate the template for no reason).

And you're welcome @dmachaj! I had some free time this weekend so I decided why not do this :)

@kennykerr
Copy link
Collaborator

If these delegates have two additional vtable slots each

This does not affect binary (vtable) layout.

Copy link
Contributor

@dmachaj dmachaj left a comment

Choose a reason for hiding this comment

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

:shipit:

@kennykerr kennykerr merged commit 953d65c into microsoft:master Jul 10, 2023
96 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It would be nice if event registrations could take weak_from_this() for shared_ptr in addition to get_weak()
3 participants