final on class vs destructor in style guide?

[Zentaro Kavanagh]

tldr; Should we update the style guide to explicitly tell you to mark a class final not just a destructor?

In the style guide we encourage people to annotate a destructor as final. This is effectively the same as marking the class final but less obvious and you get a worse error.

From here

"Explicitly annotate overrides of virtual functions or virtual destructors with exactly one of an override or (less frequently) final specifier. Do not use virtual when declaring an override. Rationale: A function or destructor marked override or final that is not an override of a base class virtual function will not compile, and this helps catch common errors. The specifiers serve as documentation; if no specifier is present, the reader has to check all ancestors of the class in question to determine if the function or destructor is virtual or not."

Here's one example in the code base where we annotate the destructor final.

In newer versions of clang there is a warning -Wfinal-dtor-non-final-class which issues a warning (currently disabled in chrome) to add final to the class. eg.

external/base/task/sequence_manager/sequence_manager_impl.cc:156:27: warning: class with destructor marked 'final' cannot be inherited from [-Wfinal-dtor-non-final-class]
  ~NativeWorkHandleImpl() final {
                          ^
external/base/task/sequence_manager/sequence_manager_impl.cc:145:28: note: mark 'base::sequence_manager::internal::SequenceManagerImpl::NativeWorkHandleImpl' as 'final' to silence this warning
class SequenceManagerImpl::NativeWorkHandleImpl : public NativeWorkHandle {

Changes that implemented this...

- Warning added to clang - https://reviews.llvm.org/D66711
- Devirtualize other methods - https://reviews.llvm.org/D66621

Should the style guide be updated to reflect this, and is it a prerequisite to cleaning up the instances in the code per this bug? And should the guidance be to annotate both, or only the class?

[Nico Weber]

https://crbug.com/999886 tracks cleaning up all code and turning on that warning.

IMHO marking final classes final at class level instead of at dtor level is a bit nicer.

I'm not sure it's nicer enough that it warrants an explicit recommendation. (As in: I could go either way.)

If someone wanted to mark all final classes final and turn on that warning, I think that'd be cool.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABO%2BUTrEPy6OF05xc9xZKpNfK-iUTnBL7%3DXP%2BEvFZ-p2qpfp1w%40mail.gmail.com.

[Zentaro Kavanagh]

Really the main difference is the error you get which is a bit better. Mostly just checking if people thought that this contradicts the guidance or is just supplementary.

// Destructor final

external/base/task/sequence_manager/sequence_manager_impl.cc:180:28: error: declaration of '~BadDerived' overrides a 'final' function
class SequenceManagerImpl::BadDerived : public SequenceManagerImpl::NativeWorkHandleImpl {};
                           ^
external/base/task/sequence_manager/sequence_manager_impl.cc:180:28: note: while declaring the implicit destructor for 'BadDerived'
external/base/task/sequence_manager/sequence_manager_impl.cc:156:3: note: overridden virtual function is here
  ~NativeWorkHandleImpl() final {
  ^

// Class final

external/base/task/sequence_manager/sequence_manager_impl.cc:180:48: error: base 'NativeWorkHandleImpl' is marked 'final'
class SequenceManagerImpl::BadDerived : public SequenceManagerImpl::NativeWorkHandleImpl {};
                                               ^
external/base/task/sequence_manager/sequence_manager_impl.cc:145:28: note: 'NativeWorkHandleImpl' declared here
class SequenceManagerImpl::NativeWorkHandleImpl final : public NativeWorkHandle {

[Peter Kasting]

On Mon, Oct 12, 2020 at 12:14 PM 'Zentaro Kavanagh' via cxx <c...@chromium.org> wrote:

Really the main difference is the error you get which is a bit better. Mostly just checking if people thought that this contradicts the guidance or is just supplementary.

I don't think there's a contradiction.  I'm with Nico: seems like if someone once to fix-and-enable this, it'd be fine, and I'm not sure any new style writings are necessary.

PK

[Karl Wiberg]

One technical point in favor of making the class rather than the destructor final: it also works on classes that don't have virtual functions. It seems much cleaner to just say

Explicitly leaf classes should be marked `final`.

instead of

Explicitly leaf classes can either be marked `final`, or have a `final` destructor in case the destructor is virtual.

(I'm a big fan of marking all sorts of classes final, since it provides the human reading the code with a valuable nonlocal clue.)

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFBWAOABdV%2BRqJRBpzpY9tN_T0FiAAU%3DbaCHxY%3DEHza0BA%40mail.gmail.com.

--

Karl Wiberg

Software Engineer

kwi...@google.com

+46 70 696 1024