Style guide update for destructors - access specifiers and virtuality

[Ryan Sleevi]

As written:
From the Google C++ style guide, which serves as the base for
Chromium's style:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Interfaces

"To make sure all implementations of the interface can be destroyed
correctly, the interface must also declare a virtual destructor (in an
exception to the first rule, this should not be pure). See Stroustrup,
The C++ Programming Language, 3rd edition, section 12.4 for details"

(This is also a reference to Stroustrup, 12.6.9, "A class with a
virtual function should have a virtual destructor §12.4.2")

Proposal:
An exemption, detailed in the Chromium style guide at
http://dev.chromium.org/developers/coding-style , based on
http://www.gotw.ca/publications/mill18.htm / "H. Sutter, More
Exceptional C++, Item 27, p168"

"A base class destructor should be either public and virtual, or
protected and nonvirtual."


Context:
Throughout the Chromium codebase, we tend to have many "Delegate" and
"Interface" type classes that declare a series of pure virtual
functions that must be implemented by derived classes. These Delegates
are then passed to some concrete class, as the way of having the lower
level and higher level code communicate, via Dependency Injection.
Many times, the lifetimes of these Delegates are defined as "Must
outlive the class they're passed to". Alternatively, and subjectively
less commonly, they're defined as "This class takes ownership of the
Delegate", with the concrete class then storing the class in
scoped_ptr<Delegate>


Reason in favor:
For situations where there is no particular ownership requirement for
the Delegate - that is, it outlives whatever class or set of classes
its passed to - I would propose that we should allow explicit,
protected, non-virtual destructors to be declared. For situations
where ownership is retained or intended to be retainable, the
destructor should be explicit, public, and virtual.

The context for this is I have been working to ensure that any
RefCounted/RefCountedThreadSafe derived class does not have an
accessible public destructor, as that can lead to double frees. In a
number of places, there are ref-counted classes implementing some
FooInterface. FooInterface may have an implicit (compiler-generated)
public destructor, or it may have a public, virtual destructor, even
though ownership of FooInterface is never taken. Occasionally, they
are protected and virtual.

By permitting protected, non-virtual destructors on these Interface
classes, we allow them to be safely implemented by ref-countable
classes, without worrying about accidental double deleting by way of
deleting via the FooInterface* (explicitly, scoped_ptr, DeleteSoon,
etc). Further, with Clang, we can then make it a compile error if any
(base) class in the ref-counted implementation has a public
destructor.


Reason against:
It's been pointed out that by ensuring Interfaces have explicit,
public, virtual destructors, it becomes easier to do dependency
injection. The class that expects an Interface can then reliably store
ownership of the Interface, if they wish, ensuring that the Interface
is always a valid pointer while the class that uses it is valid, and,
once the class that uses it goes out of scope, that the Interface is
always cleaned up.

Rather than multiply-inheriting, RefCounted classes that wish to
implement FooInterface would instead need to use the adapter pattern
to glue the Interface implementation to the ref-counted implementation
that performs the actual work. For some, this offers a better
solution, in that it resolves ambiguities regarding inheritance chains
and encourages more composition. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Multiple_Inheritance#Multiple_Inheritance
and http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheritance#Inheritance
for more on Composition-vs-Inheritance preferences.

[John Abd-El-Malek]

I don't think the reasons below are strong enough to diverge from the Google style guide. google3 must have tons of delegate interfaces as well, and if it was compelling there, they would have made an exception.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
   http://groups.google.com/a/chromium.org/group/chromium-dev

[Brett Wilson]

I think the Chromium style guide is unusably long already and I would
prefer not to add further divergence unless there's a strong reason.
Also, having a non-virtual destructor on a virtual class just looks
like an error to me (and I suspect many others), even though I
understand the point of the rule.

Brett

[Ami Fischman]

On Tue, May 22, 2012 at 1:44 PM, John Abd-El-Malek <j...@chromium.org> wrote:

I don't think the reasons below are strong enough to diverge from the Google style guide. google3 must have tons of delegate interfaces as well, and if it was compelling there, they would have made an exception.

Google style eschews multiple-inheritance (and refcounting is much rarer than in chrome code), so the need for such a proposal is much lesser there.  Just sayin'.

-a

[William Chan (陈智昌)]

I also would prefer not to add more exceptions to the Google C++ style guide. The Chromium style guide is already too long if you ask me.

FWIW, I dislike multiple inheritance and dislike our use of implementation inheritance everywhere and dislike intrusive refcounting. But I'm just being crotchety now :P

[Ryan Sleevi]

Considering that Will was among those who requested protected+non-virtual, it sounds like the general sentiment is that it's better to just keep that style guide as is and just declare destructors as virtual everywhere they're used for inheritance, regardless of their access specification.

This works for me, as it addresses the conflicting review feedback related to this point.

[Tommi]

FWIW - I like the proposal and think that a protected, non-virtual destructor explicitly states (and documents) how the interface is meant to be used in addition to forbidding public deletion.  Imho it is confusing to have a virtual destructor that must never be used as such (i.e. via the interface).

However, I also agree that the style guide is a bit long so maybe this isn't a strong enough reason.

[kris...@chromium.org]

I see that the style guide is changed, and doesn't mention destruction anymore. What is considered the best practise for interfaces?

Also, why not just have a virtual and protected destructor?

My reason for asking is that all the interfaces without a virtual destructors gives warnings on Android and I would rather fix them then turn off the warning.

[kris...@chromium.org]

Ah, sorry for the spam. I didn't see that the item could be expanded, and that a virtual destructor is still required by the style guide. Will start hunting them down :-)

[Jonathan Dixon]

On 27 December 2012 19:19, <kris...@chromium.org> wrote:

Ah, sorry for the spam. I didn't see that the item could be expanded, and that a virtual destructor is still required by the style guide. Will start hunting them down :-)

kl. 18:45:28 UTC-8 torsdag 27. desember 2012 skrev kris...@chromium.org følgende:

I see that the style guide is changed, and doesn't mention destruction anymore. What is considered the best practise for interfaces?

Also, why not just have a virtual and protected destructor?

I'd not read that gotw before, but now I have I wondered exactly that. I don't see anything there that directly explains why a protected destructor is better off being nonvirtual, when applied to classes that already have at least one other virtual method.

On the that topic, would be nice to move towards enforcing the -Wno-non-virtual-dtor warning.