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

class_loader_imp: do not call virtual function in destructor #150

Open
wants to merge 1 commit into
base: melodic-devel
Choose a base branch
from

Conversation

cwecht
Copy link
Contributor

@cwecht cwecht commented Apr 11, 2019

This fixes clang-tidys "clang-analyzer-optin.cplusplus.VirtualCall"-warning.

Also this makes more clear, which function is actually called.

@nuclearsandwich nuclearsandwich self-requested a review April 12, 2019 17:40
@nuclearsandwich
Copy link
Member

Humbly begging your forgiveness @cwecht for the tardy review. I discussed this change with @clalancette and here is what we observed:

  • The current destructor could result in a crash when the virtual function is called since the implementation is in the subclass whose destructor has already been run.
  • With this patch, any subclass which overloads getBaseClassType will not have its implementation used here.

With those observations in mind, averting a crash is highly desirable but the tradeoff is that anyone out there overriding getBaseClassType won't have their implementation used by the debug message.

We discussed making getBaseClassType private and non-virtual but that only works if no one is actually using overrides.

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.

2 participants