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

OcTreeDataNode's destructor is not virtual #70

Closed
T045T opened this issue Nov 3, 2014 · 3 comments
Closed

OcTreeDataNode's destructor is not virtual #70

T045T opened this issue Nov 3, 2014 · 3 comments
Labels

Comments

@T045T
Copy link

T045T commented Nov 3, 2014

The destructor for OcTreeDataNode (octomap/include/octomap/OcTreeDataNode.h:65) does a delete on each of the node's children.

However, children is of type OcTreeDataNode, which means the OcTreeDataNode destructor is called for children, rather than the correct subclass destructor.

This breaks horribly for me with an OcTreeDataNode subclass whose destructor needs to be called.

The obvious fix is to make OcTreeDataNode's destructor virtual - unless there's an important reason not to :)

@ahornung
Copy link
Member

ahornung commented Nov 5, 2014

The reason why there are no virtual functions in the octree nodes is to reduce memory. Declaring a virtual function will force the compiler to insert a vtable pointer, thus increasing the size of every node in the tree by 4 or 8 bytes. The nodes are not designed to be used polymorphically, all children of a node need to have the same type.

So this should be fixed in a different way, see for example the casts in getChild(i).

@ahornung ahornung added the bug label Nov 5, 2014
@T045T
Copy link
Author

T045T commented Nov 6, 2014

Thanks for the explanation - I was thinking of code reuse more than size, but obviously even 4 bytes per node are bound to make a noticeable difference at the scale octomap operates on :)

So node classes that implement their own destructor should just cast the children array to their own type (or use their own getChild() method), right?

@ahornung
Copy link
Member

ahornung commented Nov 6, 2014

Yes, that should do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants