1018323 - Add convenience macro(s) for declaring/defining private non-virtual destructor according to the XPCOM rules

Status: NEW

(Core :: XPCOM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

We should define a macro something line this:

// For XPCOM implementations that are not a base class for some other
// class, it is good practice to make the destructor non-virtual and
// private.  Then the only way to delete the object is via Release.
//
// MSVC warning C4265: Class has virtual members but destructor is not virtual
#ifdef _MSC_VER
#define NS_NON_VIRTUAL_DESTRUCTOR_IN_LINE_WITH_THE_XPCOM_SCRIPTURES(Destructor) \
  __pragma(warning(disable:4265)) Destructor
#else
#define NS_NON_VIRTUAL_DESTRUCTOR_IN_LINE_WITH_THE_XPCOM_SCRIPTURES(Destructor) Destructor
#endif

Then we would use it like this:

private:
  NS_NON_VIRTUAL_DESTRUCTOR_IN_LINE_WITH_THE_XPCOM_SCRIPTURES(~MyClass());

This would serve two benefits:

1. It would document why we're going against something normally considered a C++ best practice.
2. It would automatically suppress the MSVC warning in code that has re-enabled that warning.

Now let's commence a 1,000 message debate on the naming and the proper header file to put it in.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Daniel Holbert [:dholbert] wrote in bug 1010634 comment #24:
> (In reply to Camilo Viecco (:cviecco) from comment #23)
> > >  private:
> > > +  // For XPCOM implementations that are not a base class for some other
> > > +  // class, it is good practice to make the destructor non-virtual and
> > > +  // private.  Then the only way to delete the object is via Release.
> 
> One nit on the language here -- currently, it sounds like you're saying the
> "non-virtual" aspect is part of what gets us this
> no-deletes-outside-of-release safety benefit. But I don't think non-virtual
> really impacts that at all. Non-virtual just gives us a perf win (from not
> needing a vtable lookup at destruction time), IIUC.  So perhaps consider
> dropping the mention of "non-virtual", or reword to make it clearer that
> that's not part of the safety guarantee here.
>  
> > Would be nice to point here to the recent dev-platform thread / bug number /
> > doc
> 
> I'm hoping this pattern will be picked up across the codebase (there are
> probably thousands of classes that want this treatment).  (briansmith++ for
> adding it here!).  Given that, IMHO, it's not necessarily useful for each
> individual instance of the pattern to link to the dev-platform thread/bug#
> -- if that starts happening, it'll get too verbose.
> 
> If you like, though, feel free to use the same one-liner comment that I used
> in all the instances of this pattern that I've added:
>     // Private destructor, to discourage deletion outside of Release():
> (no pressure though)