Destructors and inheritance; risk of UB

[thomthom]

The wrapper classes uses inheritance, but their destructors are not marked as virtual:

https://github.com/TommyKaneko/Sketchup-API-C-Wrapper/blob/6879346ce7ca292962e40246e4f587020fcc9273/include/SUAPI-CppWrapper/model/Entity.hpp#L93

If one of these derived classes were allocated on the heap (using new) then a delete on a base class pointer would lead to Undefined Behaviour.

These wrappers however are wrapping opaque pointers (refs) and probably passed around as values, in which case it wouldn't be a problem. However, the interface doesn't protect against incorrect usage.

If the objects are not meant to be deleted via a base pointer then their destructor should be non-virtual and protected.

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-dtor

C.127: A class with a virtual function should have a virtual or protected destructor

Reason A class with a virtual function is usually (and in general) used via a pointer to base. Usually, the last user has to call delete on a pointer to base, often via a smart pointer to base, so the destructor should be public and virtual. Less commonly, if deletion through a pointer to base is not intended to be supported, the destructor should be protected and non-virtual; see C.35.

(Emphasis on the last paragraph is mine)

C.35: A base class destructor should be either public and virtual, or protected and non-virtual

Reason To prevent undefined behavior. If the destructor is public, then calling code can attempt to destroy a derived class object through a base class pointer, and the result is undefined if the base class’s destructor is non-virtual. If the destructor is protected, then calling code cannot destroy through a base class pointer and the destructor does not need to be virtual; it does need to be protected, not private, so that derived destructors can invoke it. In general, the writer of a base class does not know the appropriate action to be done upon destruction.

[TommyKaneko]

Thank you for catching this. Changes committed. It is correct to be a virtual and public destructor, for polymorphism.

[thomthom]

In this case, then maybe a protected non-virtual destructor is appropriate. Are there any usecases for creating any of these wrapper objects on the heap (using new)? They are essentially opaque pointers over the SU API refs (which is again opaque pointers).

Adding virtual to this introduce the vtable, eating more memory per object. If these objects are only ever used without being allocated by new - or to be more precise, without being deleted by a base pointer, then leaving it non-virtual but protected might be a more suitable option.

[TommyKaneko]

I have not found any use cases for creating wrapper objects on the heap using new. However, if I make the destructor protected non-virtual, then I have issues with Temporary Objects, which I do use.

For example, this bit of code fails to compile when I have protected Entity destructor:

Entity RubyAPI::entity_from_ruby(const RUBY_VALUE& ruby_entity) {
  SUEntityRef entity_ref = SU_INVALID;
  SU_RESULT res = SUEntityFromRuby(ruby_entity, &entity_ref);
  // ...Skip error checking....
  return Entity(entity_ref, true); // Fails to compile as a temporary object needs to have a non-protected destructor.
}

I don't know as much as yourself about C++ @thomthom, but from what I understand the additional memory cost for the vtable is not large, and the additional memory is per class and not per object ( https://stackoverflow.com/questions/1626290/c-virtual-function-table-memory-cost ). There is an additional pointer per instance to the vtable however. If that is the case, I don't think it is something to worry about too much?

If there are ways to get around the temporary objects issue but use protected destructors, I'm certainly open to avoiding virtual methods.

[thomthom]

hm... I don't see why a temporary needs a non-protected dtor. I'll have to have a closer look.

[thomthom]

Do you have an example of issues with temporary objects when the dtor is protected?