RFR: 8209345: Merge SATBMarkQueueFilter into SATBMarkQueueSet

[Thomas Schatzl]

On Mon, 2018-08-13 at 19:10 -0400, Kim Barrett wrote:
> > On Aug 13, 2018, at 6:09 AM, Thomas Schatzl <thomas.schatzl at oracle.
> > com> wrote:
> > 
> > Hi,
> > 
> > On Sun, 2018-08-12 at 22:08 -0400, Kim Barrett wrote:
> > > Please review this change that eliminates SATBMarkQueueFilter in
> > > favor
> > > of an extension to SATBMarkQueueSet.  […]
> > > 
> > > Webrev:
> > > http://cr.openjdk.java.net/~kbarrett/8209345/open.00/
> > > 
> > > Testing:
> > > Mach5 tier1-3, hs-tier4-5, in conjunction with JDK-8209346 and
> > > JDK-
> > > 8209347.
> > > Locally tier1 and hotspot_gc without those other changes.
> > > 
> > 
> >  - I think the SATBMarkQueueSet's destructor should be virtual,
> > given that there will be a second implementor with unknown
> > functionality (and in general I prefer to have virtual destructors
> > as soon there is any virtual method). Also the SATBBufferClosure's
> > one.
> 
> These destructors (and similarly for PtrQueue and PtrQueueSet) are
> protected, which is the other approach [1] to ensuring there will be
> no destructor slicing. Objects in these hierarchies are never
> destroyed via a base class pointer, so don't need a public base class
> destructor (which should indeed be virtual).
> 
> [1] "50. Make base class destructors public and virtual or protected
> and nonvirtual", C++ Coding Standards, Sutter & Alexandrescu.
> 

  this is true and good for this class in isolation, but the problem
will arise as soon as you will have descendents of this class which
make the destructor public.

At that point somebody needs to remember that the destructor needs to
be declared as virtual, and people tend to forget. We've had a few of
these issues already.

A general rule of "use a virtual destructor in the class that adds
virtual methods the first time" is simpler to remember and safer than
everybody needing to remember (dev + reviewers) to look through the
class hierarchy whether the destructor is already virtual.

Now that's not completely fail-safe either (you can still forget), but
it seems a more uncommon failure mode than what these very
knowledgeable people suggest.

Thanks,
  Thomas