Merge ~info-martin-konrad/epics-base:make-protected-dtors-non-virtual into ~epics-core/epics-base/+git/epics-base:3.15

[Martin Konrad]

[Martin Konrad (info-martin-konrad)]

Hi Jeff,
Please take a look at the following minor merge request for the existing
timer code:

https://code.launchpad.net/~info-martin-konrad/epics-base/+git/epics-base/+merge/382156

Does this make sense?

Martin

[Jeff Hill (johill-lanl)]

> Please take a look at the following minor merge request for the existing
> timer code:
>
> https://code.launchpad.net/~info-martin-konrad/epics-base/+git/epics-
> 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

[Jeff Hill (johill-lanl)]

I just now experimented with introducing one of the above changes and I immediately see warning from gcc.

base/timerQueueFix/src/libCom/timer/timer.cpp: In member function ‘virtual void Timer::destroy()’:
base/timerQueueFix/src/libCom/timer/timer.cpp:42:12: warning: deleting object of polymorphic class type ‘Timer’ which has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]
     delete this;

> gcc --version
gcc (SUSE Linux) 4.8.5

So I am not well convinced that this is a good idea. Our only cost for the pure virtual DTOR is a small increase in the VTBL size which is probably insignificant.

review: Needs Fixing

[Jeff Hill (johill-lanl)]

I will not, at this time, introduce these changes into my version. Pending obtaining compelling positives for doing this.

Our only cost for the library implementation safety of a pure virtual DTOR is the small increase in the VTBL size which is probably insignificant.

review: Needs Resubmitting

[mdavidsaver (mdavidsaver)]

I think this change is benign, though only because of the unsafe legacy design where virtual epicsTimer::destroy() take the place of destructor.

Unmerged commits

e6aaf3d... by Martin Konrad on 2020-04-14

Make the protected dtors in epicsTimer.h non-virtual

These dtors seem to be marked protected to prevent destructing
a derived object via a base-class pointer. If this dtor cannot
be called there is no need to mark it virtual. Remove "virtual"
to avoid confusion. Refer to
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-dtor-virtual
for details.

diff --git a/src/libCom/timer/epicsTimer.h b/src/libCom/timer/epicsTimer.h
index 72270f2..0839ea8 100644
--- a/src/libCom/timer/epicsTimer.h
+++ b/src/libCom/timer/epicsTimer.h
@@ -67,7 +67,7 @@ public:
     double getExpireDelay ();
     virtual void show ( unsigned int level ) const = 0;
 protected:
-    virtual ~epicsTimer () = 0; /* protected => delete() must not be called */
+    ~epicsTimer (); /* protected => delete() must not be called */
 };
 
 class epicsTimerQueue {
@@ -75,7 +75,7 @@ public:
     virtual epicsTimer & createTimer () = 0;
     virtual void show ( unsigned int level ) const = 0;
 protected:
-    epicsShareFunc virtual ~epicsTimerQueue () = 0;
+    epicsShareFunc ~epicsTimerQueue ();
 };
 
 class epicsTimerQueueActive
@@ -85,7 +85,7 @@ public:
         bool okToShare, unsigned threadPriority = epicsThreadPriorityMin + 10 );
     virtual void release () = 0; 
 protected:
-    epicsShareFunc virtual ~epicsTimerQueueActive () = 0;
+    epicsShareFunc ~epicsTimerQueueActive ();
 };
 
 class epicsTimerQueueNotify {
@@ -97,7 +97,7 @@ public:
     /* return this quantum in seconds. If unknown then return zero. */
     virtual double quantum () = 0;
 protected:
-    epicsShareFunc virtual ~epicsTimerQueueNotify () = 0;
+    epicsShareFunc ~epicsTimerQueueNotify ();
 };
 
 class epicsTimerQueuePassive