RFR: 8209345: Merge SATBMarkQueueFilter into SATBMarkQueueSet

[Kim Barrett]

> On Aug 14, 2018, at 4:54 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> 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

I disagree, but also think this RFR thread is the wrong place for this
discussion.  I've pushed the change as-is, but we can continue the
discussion elsewhere and the code can be modified if needed.