Code review comment for ~info-martin-konrad/epics-base:make-protected-dtors-non-virtual

Revision history for this message
Jeff Hill (johill-lanl) wrote :

> Please take a look at the following minor merge request for the existing
> timer code:
> base/+merge/382156
> Does this make sense?

This interface was actually one of my first forays into C++ library interface design. Today I would design the destroying part of the interface differently.

Yes I agree that their logical conclusions at this link are correct. The pure virtual destruct cant be called by users so the small overhead for its existence isn't required. It isn't harmful, but it does impose a small memory penalty, and therefore should not have been included in the original interface.

The pure virtual base destruct interface actually does protect the library implementation itself from inadvertently destroying through the base, but this is admittedly a much more tolerable risk.

The only downside to this proposal is that it is an interface change, requiring recompilation of _all_ user codes which are linked with shareable libraries. If this recompilation doesnt occur I suspect that there is real risk for run time failure depending on the compiler implementation. This is because the layout of the vtbl will certainly change. So this change is arguably appropriate for appearing in a new major release, but not into a point release.

So I agree in principal with the change, but remembering to introduce it into base at the appropriate time is the primary difficulty.

So the crux question is what version of base does this go into?

I will add these changes to my version of the timer queue assuming that we might need to remember to back them out if someone decides to back patch the heap algorithm changes into a point release.

Another consideration is that certain C++ compilers will throw warnings for _any_ class with virtual functions that does not have a pure virtual base.

review: Needs Information

« Back to merge proposal