Skip to content
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

tf2_ros: add virtual destructor to tf2_ros::BufferInterface #346

Closed
wants to merge 1 commit into from

Conversation

meyerj
Copy link
Contributor

@meyerj meyerj commented Nov 22, 2018

Class tf2_ros::BufferInterface did 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-dtor or -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                                                                                                                                                                                                                  
      ^                                                     

@tfoote
Copy link
Member

tfoote commented Nov 22, 2018

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.

The CI is pending a buildfarm upgrade: https://status.ros.org

@ahcorde
Copy link
Contributor

ahcorde commented Feb 2, 2024

This PR is targeting a EOL distribution melodic, closing this PR, feel free to reopen this against a maintained branch

@meyerj
Copy link
Contributor Author

meyerj commented Feb 3, 2024

This PR is targeting a EOL distribution melodic, closing this PR, feel free to reopen this against a maintained branch

Done: #560

@meyerj meyerj deleted the patch-2 branch February 3, 2024 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants