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

Make hub_connection always a shared_ptr #36

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BrennanConroy
Copy link
Member

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.

@@ -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
Copy link
Member

@halter73 halter73 Jun 26, 2020

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.

Copy link
Member Author

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

Copy link
Member

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?

@jwittner
Copy link

jwittner commented Mar 2, 2021

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 hub_connection in our classes (there's no default or copy constructors). We had to add a default constructor to the class in order to do so.

This change looks like the right kind of solution.

@TobyGilbert-ms
Copy link

Looks like there's a move constructor available for hub_connection which allows us to use a shared pointer. But returning a shared pointer directly from the builder would be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants