[common/point_cloud] Please re-add virtual destructor to PointCloud for safe inheritance

[dlazenby]

My colleague and I recently attempted to update to PCL 1.10.1 [skipping 1.10.0] and noticed that in this commit from Nov. 2019 the "default destructors" were removed.

I see from this discussion on the pull request that certain virtual destructors were removed because PCL did not leverage them internally.
It is this one specifically that concerns me.

My concern: If the virtual destructor is removed, and I am inheriting from PointCloud (for reasons unspecified here, but available upon request) does this not open my code up to memory leaks if I pass my DerivedCloud into an existing PCL function which takes a PointCloud and then calls something that deletes the PointCloud downcast version of my DerivedCloud?

If it is not hurting anything to leave the virtual destructor for PointCloud (or in this case re-add the virtual destructor) can the virtual destructor be re-added so that I can continue to "safely inherit" from PointCloud?

[kunaltyagi]

PointCloud has no virtual member functions, as such it should be safe to inherit from it and use it because we try hard not to be the sole owner of unknown resource (using either pointer and not deleting it or using a shared_ptr) which should ease your concern.

If you know of a place where we manage the memory manually, we cant try to rectify that.

[dlazenby]

Sorry for the delayed response. You have far more experience in how the codebase is structured and maintained than I do. I just know that I was recently burned by a non-virtual destructor inheritance case and so am therefore a bit "on edge" about such matters.

For the purposes of the code that I / my team are writing, it certainly works best for us to inherit from pcl::PointCloud and extend functionality than to rely on composition or other methods that we have previously tried. So, avoiding slicing and memory leaks while still maintaining the ability to reuse the majority of PCL functions with our inherited cloud type is of great importance to us. By inheriting, we are mainly attempting to handle various LiDAR devices and managing their intrinsic information alongside the PointCloud that they generated. There is some provision for this in the "header" field of a standard PointCloud, but not enough to be useful to us.

Most functions I have seen do in fact take an object that was created externally (not managed by the PCL library) and either use it without changing it (const ref, const pointer, etc.) or use it to build up another object which is then handed back off (again not managed by the PCL library code). If you are fairly certain that not having a virtual destructor should not cause issues moving forward with a "custom point cloud" which inherits from pcl::PointCloud then that would be satisfactory consolation for now. Thank you for your feedback on this!

[kunaltyagi]

If you are fairly certain that not having a virtual destructor should not cause issues moving forward with a "custom point cloud" which inherits from pcl::PointCloud

I think so. I'd reiterate again: If you find a place where our removal of virtual dtor causes issues, please bring the matter to our notice and we'd try to rectify it first without using virtual dtor, then by using a virtual dtor.

I was recently burned by a non-virtual destructor inheritance case

This is quite relatable. But the fact remains: Virtual dtor make all users pay a cost which we'd like to avoid, as long as possible. 😄

Closing the issue (🤞)