Stiffstream / restinio

Cross-platform, efficient, customizable, and robust asynchronous HTTP(S)/WebSocket server C++ library with the right balance between performance and ease of use
Other
1.15k stars 93 forks source link

LGTM pass by value #41

Closed Milerius closed 5 years ago

Milerius commented 5 years ago

Hello i followed the example from the markdown for creating a request, but now i got this warning from LGTM security services:

Capture d’écran 2019-08-07 à 19 10 53

Can we pass the router as const reference, or it's always as copy (like your example)

Milerius commented 5 years ago

I'm looking the sample and we effectively copy each time the route_params_t, we can probably consider to pass it as const reference, or rvalue reference if it's created only for this function, no ?

88bytes seem's a lot. If it's was only a little pod i will agree with passing it by copy

eao197 commented 5 years ago

It's just an example and the form:

[](auto req, auto param) {...}

was used just to make the example more compact and readable. You can safely change the code that way:

[](const auto & req, const auto & params) {...}

In fact, somewhere deep inside RESTinio code like that is used for calling a request handler:

request_handle_t request{...};
...
router::router_params_t params{...};
...
// Calling a request_handler.
auto user_request_handler = find_handler(request, params);
if(user_request_handler)
   user_request_handler(handle, params);

So if your lambda (or functional object) accept arguments by const references instead there won't be any additional copies.

Milerius commented 5 years ago

But the req is a shared_ptr already no ?

Milerius commented 5 years ago

Thank's for the information, I was just following the readme without asking myself

eao197 commented 5 years ago

Yes. But RESTinio has to have a copy of req inside until request handler works.

Milerius commented 5 years ago

Can i consider:

http_router->http_get("/api/v1/getprice", [this](auto&&... params)
        {
            return this->price_rest_callbook.get_price(std::forward<decltype(params)>(params)...);
        });
eao197 commented 5 years ago

You can try. I didn't try that way myself.

Milerius commented 5 years ago

You can try. I didn't try that way myself.

Work's like a charm thank's for your patience and help.

eao197 commented 5 years ago

@Milerius No problems. I hope you'll enjoy using RESTinio.

Milerius commented 5 years ago

@eao197 I enjoy and i hope you will ask for adding restinio to: https://www.techempower.com/benchmarks/

I'm looking for it.

eao197 commented 5 years ago

We no longer play such marketing games. They take too much time and don't bring money, but we have to pay bills. So it is better to spend our time and our resources to do something more useful like adding new features to RESTinio or help users with their issues.

Milerius commented 5 years ago

@eao197 Ok we use your framework at @KomodoPlatform https://github.com/KomodoPlatform https://komodoplatform.com

We use a different library for restclient in C++, because i dont think with restinio we can do some client call like : RestClient::get(final_uri);

We will let you know when the project is finish and in production ! (it's open source free project)

Milerius commented 5 years ago

And the project that is using restinio: https://github.com/KomodoPlatform/antara-makerbot

eao197 commented 5 years ago

i dont think with restinio we can do some client call like : RestClient::get(final_uri);

Yes. RESTinio is a server-side only library.

We will let you know when the project is finish and in production !

Thank!

eao197 commented 5 years ago

And the project that is using restinio: https://github.com/KomodoPlatform/antara-makerbot

Can I ask you to change a sentence in your "Acknowledgments" section? RESTinio is the result of teamwork, and Nicolai Grodzitski is the author of the significant part of RESTinio. So it is better to specify "StiffStream company" instead of just eao197.

Milerius commented 5 years ago

Of course ! (done)

eao197 commented 5 years ago

this->price_rest_callbook.get_price(std::forward<decltype(params)>(params)...);

It seems that the usage of std::bind can be a more concise alternative:

http_router->http_get("/api/v1/getprice",
    std::bind(&your_callbook_class::get_price, &price_rest_callbook, _1, _2);

where your_callbook_class is the type of price_rest_callbook object.