Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closes #1446 #1448

Merged
merged 1 commit into from
Aug 1, 2019
Merged

Closes #1446 #1448

merged 1 commit into from
Aug 1, 2019

Conversation

hsutter
Copy link
Contributor

@hsutter hsutter commented Jun 16, 2019

This PR affirms that all virtual functions, including destructors, should be declared exactly one of virtual, override, or final.

Of course a virtual destructor is a virtual function: It behaves polymorphically, and it has a vtable entry that can be overwritten == overridden in a derived class exactly the same as any other derived virtual override. See also [class.virtual]/7: "Even though destructors are not inherited, a destructor in a derived class overrides a base class destructor declared virtual; see [class.dtor] and [class.free]."

However, the following exception text currently appears in C.128:

If a base class destructor is declared virtual, one should avoid declaring derived class destructors virtual or override. Some code base and tools might insist on override for destructors, but that is not the recommendation of these guidelines.

... with rationale in #721, but this exception is (a) not well-founded, and (b) inconsistent with the Guidelines' practice in other examples and with the rationale a few lines earlier for C.128 itself.

Re (a):

  • The exception is overly broad: The rationale given for this exception is entirely against marking destructors override (not virtual). So clearly the exception to write neither keyword is too broad: At most, the exception should be to write virtual rather than override. (Additionally, I think it's clear that final should be allowed, and it is inconsistent to recommend writing one of the three virtual control keywords and not the other two.)

  • Explicit virtual is primarily for class users, not class authors: The arguments given in C.128: Should destructors be marked "override"? #721 favoring this exception are from the viewpoint of the implementation of the function (even then, the arguments are debatable and debated). But virtual, override, and final are primarily for the far larger audience of class users and call sites of the function, for whom of course we should document each declared function that is polymorphic, especially the destructor -- this tells calling code whether the function is safe to call through a (smart or built-in) pointer or reference to base, which will nearly always be the case for such types. We should not make the reader of the code go away to look in the base classes to figure out whether a function declared in this class is virtual or not -- the reason this Item exists is primarily to avoid that implicit virtual antipattern via convention and automated enforcement. For class users, all virtual functions including destructors are equally polymorphic.

Re (b):

  • The Guidelines already don't follow this. For instance, two Items later (in C.130) we have this example that correctly uses override:
virtual ~D() override;

... though per C.128 it should not also specify virtual (also fixed in this PR).

  • Finally, the exception also contradicts the rationale given earlier in the same Item.

This PR affirms that all virtual functions, *including destructors*,
should be declared exactly one of `virtual`, `override`, or `final`, and
takesa pass through the document to make the examples and guidance
consistent with that.

Of course a virtual destructor is a virtual function: It behaves
polymorphically, and it has a vtable entry that can be overwritten ==
overridden in a derived class exactly the same as any other derived
virtual override. See also [class.virtual]/7: "Even though destructors
are not inherited, a destructor in a derived class overrides a base
class destructor declared virtual; see [class.dtor] and [class.free]."

However, the following exception text currently appears in C.128:

> If a base class destructor is declared `virtual`, one should avoid
declaring derived class destructors  `virtual` or `override`. Some code
base and tools might insist on `override` for destructors, but that is
not the recommendation of these guidelines.

... but this exception is (a) not well-founded, and (b) inconsistent
with the Guidelines' practice in other examples and with the rationale a
few lines earlier for C.128 itself.

Re (a):

- The exception is overly broad: The rationale given for this exception
is entirely against marking destructors `override` (not `virtual`). So
clearly the exception to write neither keyword is too broad: At most,
the exception should be to write `virtual` rather than `override`.
- Explicit `virtual` is primarily for class users, not class authors:
The arguments given in #721 favoring this exception are from the
viewpoint of the implementation of the function (even then, the
arguments are debatable and debated). But `virtual`, `override`, and
`final` are primarily for the far larger audience of *class users and
call sites* of the function, for whom of course we should document each
declared function that is polymorphic, *especially* the destructor --
this tells calling code whether the function is safe to call through a
(smart or built-in) pointer or reference to base, which will nearly
always be the case for such types. We should not make the reader of the
code go way to look in the base classes to figure out whether a function
declared in this class is virtual or not -- the reason this Item exists
is primarily to avoid that implicit virtual antipattern via convention
and automated enforcement. For class users, all virtual functions
including destructors are equally polymorphic.

Re (b): The Guidelines already don't follow this. For instance, two
Items later (in C.130) we have this example that correctly uses
`override`:

~~~
virtual ~D() override;
~~~

... though per C.128 it should not also specify `virtual` (also fixed in
this PR).

Finally, the exception also contradicts the rationale given earlier in
the same Item.
@hsutter hsutter self-assigned this Jun 17, 2019
@hsutter
Copy link
Contributor Author

hsutter commented Aug 1, 2019

Editors call: We're going to accept this PR to revert back to the previous status quo of no exception for the destructor. We are open to seeing if there is editors' consensus for this special rule for the destructor, if someone wants to promote that please create a new issue proposing reinstating this wording.

@hsutter hsutter merged commit 5fdfb20 into master Aug 1, 2019
drbenmorgan added a commit to drbenmorgan/Bayeux that referenced this pull request Oct 9, 2020
Use override consistently for overriden virtual functions. Use tidy
and ISO C++ Core Guideline C.128 so that destructors are treated
the same way. See:

- http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-override
- isocpp/CppCoreGuidelines#1448
- isocpp/CppCoreGuidelines#1446
- isocpp/CppCoreGuidelines#721 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant