-
Notifications
You must be signed in to change notification settings - Fork 72
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
Make hub_connection always a shared_ptr #36
base: main
Are you sure you want to change the base?
Conversation
@@ -17,17 +17,6 @@ namespace signalr | |||
: m_pImpl(hub_connection_impl::create(url, trace_level, log_writer, http_client, websocket_factory)) | |||
{} | |||
|
|||
hub_connection::hub_connection(hub_connection&& rhs) noexcept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weren't we going to turn hub_connection_impl into hub_connection? Otherwise, we're essentially returning a share_ptr<shared_ptr<real_hub_connection> >
. While not necessarily wrong, I don't see the point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave a little explanation in the initial PR comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about replacing the hub_connection_impl and making hub_connection contain all the logic. However, that would require including a lot of internal types in the public header and all sorts of ugliness.
There has to be some way to get rid of the redundant share_ptr without exposing internal types, no?
This change would be great @BrennanConroy. We've recently got this client working in Unreal, but the current API makes it difficult to store a This change looks like the right kind of solution. |
Looks like there's a move constructor available for |
454f88f
to
a0cf783
Compare
We talked about replacing the hub_connection_impl and making hub_connection contain all the logic. However, that would require including a lot of internal types in the public header and all sorts of ugliness.