[GPU] Use C++11 std::atomic instead of non-standard extensions#4807
[GPU] Use C++11 std::atomic instead of non-standard extensions#4807mvieth merged 11 commits intoPointCloudLibrary:masterfrom
std::atomic instead of non-standard extensions#4807Conversation
kunaltyagi
left a comment
kunaltyagi
left a comment
There was a problem hiding this comment.
We can also make the default value of refcount 0 to align more with shared_ptr, but not in this PR
kunaltyagi
changed the title
std::atomic instead of non-standard extensions
kunaltyagi
added
changelog: deprecation
|
Thank you so much, Kunal, for the review! As always, all the comments are very helpful and I will incorporate them! |
|
@FabianSchuetze What is the status here? Are you still working on this pull request? |
|
I beg your pardon @mvieth for the radio silence & thank you for pinging me. I changed jobs and moved. That should not have prevented me from continuing to work on this but it did. I made another commit and look forward to the feedback! |
|
|
||
| #include <pcl/gpu/containers/device_memory.h> | ||
| #include <pcl/gpu/utils/safe_call.hpp> | ||
| #include <pcl/pcl_macros.h> |
There was a problem hiding this comment.
Is this included for PCL_DEPRECATED or something else? It is always nice to add an explanation like ...macros.h> // for PCL_DEPRECATED.
Is it okay to assume that HAVE_CUDA is somehow included or would it be safer to explicitly include pcl_config.h?
There was a problem hiding this comment.
Thanks for the comment, Markus! I have added the explanation and think this is a nice addition indeed - thanks. I am not sure whether to include pcl_config.h. Do you know what common practice for PCL is?
There was a problem hiding this comment.
I would say pcl_config.h should be included because HAVE_CUDA is used in line 45 and it is defined in pcl_config.h, so it should be best to include it directly
There was a problem hiding this comment.
I have tried to add the header, as you suggested, @mvieth . Thanks for pointing this out!
larshg
left a comment
larshg
left a comment
There was a problem hiding this comment.
I have run the unit test we have enabled for GPUl, so hopefully they cover the device_memory usage 😄
kunaltyagi
left a comment
kunaltyagi
left a comment
There was a problem hiding this comment.
I have run the unit test
Macro is in cpp file
- Ci is green
No objections from my side
…ntCloudLibrary#4807) * use c++11 atomics * missing linebreak * address PR comments * should compile now * improved fetch_sub * init * address comments for PR * address comments for PR, again * fixed assignment operator and annotated include * added include for HAVE_CUDA * correct order
4 participants
As mentioned in #3987 we can use the memory model of c++11 to replace (outdated) cv code. This PR tries to implement the suggestions in the issue. I always find the atomic reference counting a bit strange, but it is implemented the same way in the std::shared_ptr, and I think it's good to stick with it.
I am looking forward to any suggestions and comments!