Skip to content
This repository has been archived by the owner on Jun 12, 2018. It is now read-only.

remove unused captures in lambdas #120

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

remove unused captures in lambdas #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions http_examples.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,13 @@ int main() {

//GET-example for the path /match/[number], responds with the matched string in path (number)
//For instance a request GET /match/123 will receive: 123
server.resource["^/match/([0-9]+)$"]["GET"]=[&server](shared_ptr<HttpServer::Response> response, shared_ptr<HttpServer::Request> request) {
server.resource["^/match/([0-9]+)$"]["GET"]=[](shared_ptr<HttpServer::Response> response, shared_ptr<HttpServer::Request> request) {
string number=request->path_match[1];
*response << "HTTP/1.1 200 OK\r\nContent-Length: " << number.length() << "\r\n\r\n" << number;
};

//Get example simulating heavy work in a separate thread
server.resource["^/work$"]["GET"]=[&server](shared_ptr<HttpServer::Response> response, shared_ptr<HttpServer::Request> /*request*/) {
server.resource["^/work$"]["GET"]=[](shared_ptr<HttpServer::Response> response, shared_ptr<HttpServer::Request> /*request*/) {
thread work_thread([response] {
this_thread::sleep_for(chrono::seconds(5));
string message="Work done";
Expand Down
4 changes: 2 additions & 2 deletions https_examples.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,13 @@ int main() {

//GET-example for the path /match/[number], responds with the matched string in path (number)
//For instance a request GET /match/123 will receive: 123
server.resource["^/match/([0-9]+)$"]["GET"]=[&server](shared_ptr<HttpsServer::Response> response, shared_ptr<HttpsServer::Request> request) {
server.resource["^/match/([0-9]+)$"]["GET"]=[](shared_ptr<HttpsServer::Response> response, shared_ptr<HttpsServer::Request> request) {
string number=request->path_match[1];
*response << "HTTP/1.1 200 OK\r\nContent-Length: " << number.length() << "\r\n\r\n" << number;
};

//Get example simulating heavy work in a separate thread
server.resource["^/work$"]["GET"]=[&server](shared_ptr<HttpsServer::Response> response, shared_ptr<HttpsServer::Request> /*request*/) {
server.resource["^/work$"]["GET"]=[](shared_ptr<HttpsServer::Response> response, shared_ptr<HttpsServer::Request> /*request*/) {
thread work_thread([response] {
this_thread::sleep_for(chrono::seconds(5));
string message="Work done";
Expand Down
3 changes: 2 additions & 1 deletion server_http.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ namespace SimpleWeb {

///Use this function if you need to recursively send parts of a longer message
void send(const std::shared_ptr<Response> &response, const std::function<void(const boost::system::error_code&)>& callback=nullptr) const {
boost::asio::async_write(*response->socket, response->streambuf, [this, response, callback](const boost::system::error_code& ec, size_t /*bytes_transferred*/) {
Copy link
Owner

Choose a reason for hiding this comment

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

response is needed here to keep the object alive until async_write is finished.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds error-prone.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, but it has to be done somewhere since Boost.Asio is still quite low-level and does not support smart pointer parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated my pull-request. (added a comment, see below)

boost::asio::async_write(*response->socket, response->streambuf, [response, callback](const boost::system::error_code& ec, size_t /*bytes_transferred*/) {
(void) response; // response is not used here, but needs to captured to keep it alive because async_write is using response->*-fields
Copy link
Owner

Choose a reason for hiding this comment

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

I would like not to have this line here. I think there is a comment somewhere that explains this, but not sure if this needs to be explained for every async call, as its already stated in the Boost.Asio reference. The reason response is captured is that its streambuf object (and the socket of course) needs to be kept alive. Sorry for my bad explanation above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it only the comment which disturbs you or also the (void) response;?

Copy link
Owner

Choose a reason for hiding this comment

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

Mostly it's the (void) response;.

Copy link
Contributor Author

@pboettch pboettch Apr 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Not having the (void) reponse; will lead to a warning in (at least) future versions of clang of this kind:

http/Simple-Web-Server/https_examples.cpp:96:41: warning: lambda capture 'server' is not used [-Wunused-lambda-capture]
    server.resource["^/work$"]["GET"]=[&server](shared_ptr<HttpsServer::Response> response, shared_ptr<HttpsServer::Request> /*request*/) {

(-Wunused-lambda-capture is activated with -Wall or -Wextra)

Copy link
Owner

Choose a reason for hiding this comment

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

That is a good argument actually, but not sure if -Wunused-lambda-capture should be activated with -Wall or -Wextra. It might be an oversight by the clang++ developers, since using captures to prolong an shared_ptr's lifetime should be a valid use case, especially together with Boost.Asio. Not sure if the asio proposal to the C++ standard solves this in a different way. I'll have a look at it Tomorrow.

Copy link
Owner

Choose a reason for hiding this comment

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

See for instance http://www.boost.org/doc/libs/1_63_0/doc/html/boost_asio/example/cpp11/echo/async_tcp_echo_server.cpp. Compiling this example would also result in the same warnings with -Wall -Wextra enabled on newer clang++ (if one of these two flags enable -Wunused-lambda-capture).

if(callback)
callback(ec);
});
Expand Down
3 changes: 2 additions & 1 deletion tests/io_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ int main() {
*response << "HTTP/1.1 200 OK\r\nContent-Length: " << content_stream.tellp() << "\r\n\r\n" << content_stream.rdbuf();
};

server.resource["^/match/([0-9]+)$"]["GET"]=[&server](shared_ptr<HttpServer::Response> response, shared_ptr<HttpServer::Request> request) {

server.resource["^/match/([0-9]+)$"]["GET"]=[](shared_ptr<HttpServer::Response> response, shared_ptr<HttpServer::Request> request) {
string number=request->path_match[1];
*response << "HTTP/1.1 200 OK\r\nContent-Length: " << number.length() << "\r\n\r\n" << number;
};
Expand Down