-
Notifications
You must be signed in to change notification settings - Fork 221
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
Add missing virtual destructors #553
Conversation
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
rviz_rendering/include/rviz_rendering/objects/point_cloud_renderable.hpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Linter failures were fixed and retested on Linux, besides that all the jobs were greenish. |
I guess so, needs a second review. I just made a release of rviz, but I can roll another. |
I believe this one also does not break API (adding the virtual destructor could be seen as that, but it's a corner case because no one who was calling it before would now have an issue, but I'm open to being convinced otherwise on that one). |
I don't think it breaks API, but it breaks ABI. So, it can't be backported in the future. |
Yeah, sorry I wasn't suggesting we push it for the patch release, I just was sort of commenting on how the version number would change (also low risk for (re)testing). |
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.
LGTM
I'm okay with adding this to the initial Foxy release.
Same idea as ros2/rclcpp#1149.
There's an "overloaded virtual" warning that I'm ignoring in order to avoid breaking API now.
We should fix that in the future.