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.

Sign up for GitHub

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
Open

Make hub_connection always a shared_ptr #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3eaa50c
Make hub_connection always a shared_ptr
BrennanConroy Jun 25, 2020
a0cf783
fix
BrennanConroy Jun 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions include/signalrclient/hub_connection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,10 @@ namespace signalr

SIGNALRCLIENT_API ~hub_connection();

hub_connection(const hub_connection&) = delete;

hub_connection(const hub_connection& rhs) = delete;
hub_connection& operator=(const hub_connection&) = delete;

SIGNALRCLIENT_API hub_connection(hub_connection&&) noexcept;

SIGNALRCLIENT_API hub_connection& operator=(hub_connection&&) noexcept;
hub_connection(hub_connection&&) = delete;
hub_connection& operator=(hub_connection&&) = delete;

SIGNALRCLIENT_API void __cdecl start(std::function<void(std::exception_ptr)> callback) noexcept;
SIGNALRCLIENT_API void __cdecl stop(std::function<void(std::exception_ptr)> callback) noexcept;
Expand Down
2 changes: 1 addition & 1 deletion include/signalrclient/hub_connection_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ namespace signalr
SIGNALRCLIENT_API hub_connection_builder& with_messagepack_hub_protocol();
#endif

SIGNALRCLIENT_API hub_connection build();
SIGNALRCLIENT_API std::shared_ptr<hub_connection> build();
private:
hub_connection_builder(const std::string& url);

Expand Down
18 changes: 9 additions & 9 deletions samples/HubConnectionSample/HubConnectionSample.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ class logger : public signalr::log_writer
}
};

void send_message(signalr::hub_connection& connection, const std::string& message)
void send_message(std::shared_ptr<signalr::hub_connection> connection, const std::string& message)
{
std::vector<signalr::value> args { std::string("c++"), message };

// if you get an internal compiler error uncomment the lambda below or install VS Update 4
connection.invoke("Send", args, [](const signalr::value& value, std::exception_ptr exception)
connection->invoke("Send", args, [](const signalr::value& value, std::exception_ptr exception)
{
try
{
Expand All @@ -50,17 +50,17 @@ void send_message(signalr::hub_connection& connection, const std::string& messag

void chat()
{
signalr::hub_connection connection = signalr::hub_connection_builder::create("http://localhost:5000/default")
.with_logging(std::make_shared <logger>(), signalr::trace_level::verbose)
std::shared_ptr<signalr::hub_connection> connection = signalr::hub_connection_builder::create("http://localhost:5000/default")
.with_logging(std::make_shared<logger>(), signalr::trace_level::verbose)
.build();

connection.on("Send", [](const std::vector<signalr::value>& m)
connection->on("Send", [](const std::vector<signalr::value>& m)
{
std::cout << std::endl << m[0].as_string() << std::endl << "Enter your message: ";
});

std::promise<void> task;
connection.start([&connection, &task](std::exception_ptr exception)
connection->start([&connection, &task](std::exception_ptr exception)
{
if (exception)
{
Expand All @@ -77,20 +77,20 @@ void chat()
}

std::cout << "Enter your message:";
while (connection.get_connection_state() == signalr::connection_state::connected)
while (connection->get_connection_state() == signalr::connection_state::connected)
{
std::string message;
std::getline(std::cin, message);

if (message == ":q" || connection.get_connection_state() != signalr::connection_state::connected)
if (message == ":q" || connection->get_connection_state() != signalr::connection_state::connected)
{
break;
}

send_message(connection, message);
}

connection.stop([&task](std::exception_ptr exception)
connection->stop([&task](std::exception_ptr exception)
{
try
{
Expand Down
11 changes: 0 additions & 11 deletions src/signalrclient/hub_connection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,6 @@ namespace signalr
: m_pImpl(hub_connection_impl::create(url, std::move(hub_protocol), trace_level, log_writer, http_client_factory, websocket_factory, skip_negotiation))
{}

hub_connection::hub_connection(hub_connection&& rhs) noexcept
Copy link
Member

@halter73 halter73 Jun 26, 2020 •
edited
Loading

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?

: m_pImpl(std::move(rhs.m_pImpl))
{}

hub_connection& hub_connection::operator=(hub_connection&& rhs) noexcept
{
m_pImpl = std::move(rhs.m_pImpl);

return *this;
}

// Do NOT remove this destructor. Letting the compiler generate and inline the default dtor may lead to
// undefined behavior since we are using an incomplete type. More details here: http://herbsutter.com/gotw/_100/
hub_connection::~hub_connection()
Expand Down
4 changes: 2 additions & 2 deletions src/signalrclient/hub_connection_builder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ namespace signalr
}
#endif

hub_connection hub_connection_builder::build()
std::shared_ptr<hub_connection> hub_connection_builder::build()
{
#ifndef USE_CPPRESTSDK
if (m_http_client_factory == nullptr)
Expand All @@ -115,6 +115,6 @@ namespace signalr
hub_protocol = std::unique_ptr<json_hub_protocol>(new json_hub_protocol());
}

return hub_connection(m_url, std::move(hub_protocol), m_log_level, m_logger, m_http_client_factory, m_websocket_factory, m_skip_negotiation);
return std::shared_ptr<hub_connection>(new hub_connection(m_url, std::move(hub_protocol), m_log_level, m_logger, m_http_client_factory, m_websocket_factory, m_skip_negotiation));
}
}
Loading