ros
tf2_ros: add virtual destructor to tf2_ros::BufferInterface
Same as https://github.com/ros/geometry2/pull/346, targeting noetic-devel:
Class
tf2_ros::BufferInterfacedid not define a virtual destructor, but has virtual methods. This is dangerous because it may lead to memory leaks if instances are deleted using a pointer to the base class (reference: https://stackoverflow.com/questions/1123044/when-should-your-destructor-be-virtual).Clang and GCC with
-Wnon-virtual-dtoror-Weffc++warn about this:include/tf2_ros/buffer_interface.h:48:7: warning: 'tf2_ros::BufferInterface' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor] class BufferInterface ^
I assume this comment still applies:
This looks right, but I believe it will break ABI so we'll need to evaulate whether an ABI change or memory leak is worse. In general since we rebuild everything right now I think the ABI change is likely less of an issue.
So the patch cannot be merged into noetic-devel without breaking the ABI, but should probably have been considered for a new distribution in between. Overall, the severity of the potential memory leak is low, because likely in most applications tf buffers will be created and destroyed once, and not deleted repeatedly via a tf2_ros::BufferInterface pointer. So it may be fine to keep it as-is and close the PR, given that there will be no future ROS 1 release anymore?
By the way, for ROS 2 the same issue has been addressed in https://github.com/ros2/geometry2/commit/5178d48b946bdd60941bc8540eb611076974cd8d.
What is the policy of breaking the ABI on branch noetic-devel? If ABI breaking changes are not accepted anyway, for a good reason, then maybe I should re-submit this to https://github.com/ros-o/geometry2?