-
Notifications
You must be signed in to change notification settings - Fork 3
improvements #1
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
base: main
Are you sure you want to change the base?
improvements #1
Conversation
Factor temporary std::string instance to separate variable as this prolongs its lifetime long enough for result of .c_str to survive. Otherwise printf could obtain pointer to already destructed string. For more information consult GotW #88 at [1]. [1] https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/
Sometimes it is difficult to distinguish between
a function call and object construction. It is better
to use {} for object construction for this reason.
Captures in lambdas are const by default. So in order to be able to move from them, we mark the entire lambda as mutable. Thus, member variables in our closure object are no longer const. Also, using a variable multiple times on the same line and one of the uses is modifying the variable is a no-no. Extract the pointer out of the unique_ptr before performing modifying operation.
f9e21e9 to
36efa17
Compare
|
I see what you are getting at however... error.message().c_str() is passed immediately to printf(). In C++, the temporary std::string created by error.message() lives until the end of the full expression, which is the call to printf. Therefore, the std::string returned by message() remains valid for the duration of the printf call, and its .c_str() is safe to use in this context. Agree? |
|
fully agree with the std::unique_ptr solution it is much easier to see the problem when using |
|
there is another bug, need to restrict number of bytes written to number of bytes read: eg like this auto req_ptr2 = request.get(); // need to cache ptr to prevent segfault (arg eval order)
asio::async_write(req_ptr2->socket, asio::buffer(req_ptr2->buffer.data(), bytes_transferred), ...
I noticed the problem when using this for the client: netcat localhost 6970 < /dev/random
in that case the bytes sent is always the full buffer when the bytes read is much less.. so it was sending a lot of garbage |
|
Oh, my bad, in this case the temporary string survives long enough. But in general, this it gotcha, the function might store the pointer for later use. About lambdas and unique pointers. Yeah, sad story. It might be better to capture plain old pointer instead and avoid the mutable keyword. Using .release() on a line just before the capture and constructing fresh new unique pointer on first line inside the lambda passing the plain pointer as an argument. Might not be idiomatic C++ but readable and understandable by a C professional. |
yeah fully agree.. |
Avoid footgun.
Factor temporary std::string instance to separate variable
as this prolongs its lifetime long enough for result of
.c_str to survive. Otherwise printf could obtain pointer
to already destructed string.
For more information consult GotW #88 at [1].
[1] https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/
Use {} instead of () for object construction.
Sometimes it is difficult to distinguish between
a function call and object construction. It is better
to use {} for object construction for this reason.
std::unique_ptr baby
Captures in lambdas are const by default. So in order to
be able to move from them, we mark the entire lambda
as mutable. Thus, member variables in our closure object
are no longer const. Also, using a variable multiple times
on the same line and one of the uses is modifying the
variable is a no-no. Extract the pointer out of the unique_ptr
before performing modifying operation.