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

Add missing virtual destructors #553

Merged
merged 4 commits into from
Jun 2, 2020
Merged

Conversation

ivanpauno
Copy link
Member

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.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added bug Something isn't working in review Waiting for review (Kanban column) labels Jun 1, 2020
@ivanpauno ivanpauno requested review from wjwwood and hidmic June 1, 2020 19:56
@ivanpauno ivanpauno self-assigned this Jun 1, 2020
@ivanpauno
Copy link
Member Author

Jobs config:

  • build: --packages-up-to rviz2 rviz_default_plugins rviz_rendering rviz_rendering_tests rviz_visual_testing_framework
  • test: --packages-select rviz2 rviz_default_plugins rviz_rendering rviz_rendering_tests rviz_visual_testing_framework

Jobs:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

Retriggering linux job to confirm linters are happy:

  • Linux Build Status

@ivanpauno
Copy link
Member Author

Linter failures were fixed and retested on Linux, besides that all the jobs were greenish.
@jacobperron @wjwwood is this gtg?

@wjwwood
Copy link
Member

wjwwood commented Jun 2, 2020

I guess so, needs a second review. I just made a release of rviz, but I can roll another.

@wjwwood
Copy link
Member

wjwwood commented Jun 2, 2020

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).

@ivanpauno
Copy link
Member Author

I don't think it breaks API, but it breaks ABI. So, it can't be backported in the future.

@wjwwood
Copy link
Member

wjwwood commented Jun 2, 2020

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).

Copy link
Member

@jacobperron jacobperron left a 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.

@wjwwood wjwwood merged commit 6f8d01d into ros2 Jun 2, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/add-missing-virtual-dtor branch June 2, 2020 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants