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

LGTM pass by value #41

Closed
Milerius opened this issue Aug 7, 2019 · 17 comments
Closed

LGTM pass by value #41

Milerius opened this issue Aug 7, 2019 · 17 comments

Comments

@Milerius
Copy link

Milerius commented Aug 7, 2019

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
Copy link
Author

Milerius commented Aug 7, 2019

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

eao197 commented Aug 7, 2019

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
Copy link
Author

Milerius commented Aug 7, 2019

But the req is a shared_ptr already no ?

@Milerius
Copy link
Author

Milerius commented Aug 7, 2019

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

@eao197
Copy link
Member

eao197 commented Aug 7, 2019

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

@Milerius
Copy link
Author

Milerius commented Aug 7, 2019

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

eao197 commented Aug 7, 2019

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

@Milerius
Copy link
Author

Milerius commented Aug 7, 2019

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

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

@Milerius Milerius closed this as completed Aug 7, 2019
@eao197
Copy link
Member

eao197 commented Aug 7, 2019

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

@Milerius
Copy link
Author

Milerius commented Aug 7, 2019

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

I'm looking for it.

@eao197
Copy link
Member

eao197 commented Aug 7, 2019

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
Copy link
Author

Milerius commented Aug 7, 2019

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

Milerius commented Aug 7, 2019

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

@eao197
Copy link
Member

eao197 commented Aug 7, 2019

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

eao197 commented Aug 7, 2019

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
Copy link
Author

Milerius commented Aug 7, 2019

Of course ! (done)

@eao197
Copy link
Member

eao197 commented Aug 7, 2019

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.

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

No branches or pull requests

2 participants