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
Change particle postprocessor writer from raw pointers to unique pointers #5223
Change particle postprocessor writer from raw pointers to unique pointers #5223
Conversation
source/postprocess/particles.cc
Outdated
background_thread | ||
= std::thread([&]() | ||
{ | ||
writer (filename, temporary_output_location, file_contents); | ||
writer (filename, temporary_output_location, *file_contents); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work. You start a new thread, and that thread has a reference to the std::unique_ptr
object file_contents
. But by the time the background thread dereferences it, (i) the string the pointer points to may already have been de-allocated, and (ii) the std::unique_ptr
itself may no longer be a valid object. You have to capture everything you need here by copy, and you need to take over ownership of the pointer. In visualization.cc, this looks as follows:
= std::thread([ my_filename = std::move(filename),
my_temporary_output_location = temporary_output_location,
my_file_contents = std::move(file_contents)]()
{
writer (my_filename, my_temporary_output_location, *my_file_contents);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will note that this was already wrong for the filename
and temporary_output_location
variables. I think we only got lucky by the fact that the thread immediately calls writer
, which copies these objects because these arguments are passed by value. This copy must have happened quickly enough that the main function did not already exit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, oke. I did see that it was done slightly different in visualization.cc, but I wanted to keep the code as little as possible while introducing the unique_ptr. I will now just make it the same as there (which I probably should just have done from the start)
6f7f5d2
to
b4e7edd
Compare
fixed it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, great, thank you!
Based on comments in #4979. This brings the writer more in line with the writer in visualizer.cc