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

C.128 Doesn't provide reasoning for rule exception with destructors #832

Closed
adambadura opened this issue Jan 27, 2017 · 10 comments
Closed
Assignees

Comments

@adambadura
Copy link

C.128 Virtual functions should specify exactly one of virtual, override, or final makes an exception for destructors:

If a base class destructor is declared virtual, derived class destructors should neither be declared virtual nor override.

However no reasoning for this is provided.

I think it would be better to make it a separate paragraph, perhaps with some emphasis so that it clearly stands out as an exception to the general rule. And also provide there justification for such exception.

@cubbimew
Copy link
Member

related #721

@adambadura
Copy link
Author

What is the procedure here? Should I propose rewording to that point or someone else is reviewing such issues and applying (or not) changes to the text?

@strpbrk
Copy link

strpbrk commented Mar 13, 2017

This is bothering me, too. I have also read discussions about it, but mostly only saw difference in opinions.

Given that C.35 allows (and requires) a non-virtual protected destructor to be used, when I see a destructor declaration such as ~Derived(), I would like to know whether it is virtual or not without having to look at the base class.

And what's even more important, if I switch the ~Base to protected non-virtual, the compiler won't flag anything on the ~Derived. If there was override, the derived destructor would be flagged. (In this case maybe Derived is yet another abstract class that augments the Base, and I want to use it with std::unique_ptr<Derived>).

Before someone says that pure abstract classes (interfaces) should only have public virtual destructors, I would like to point again at C.35. Does destruction always have to be exposed in an interface?

// Whatever the interface here may be...
Article * Newspaper::get_article();
Article & Newspaper::get_article();
shared_ptr<Article> Newspaper::get_article();

class Article {
public:
    // Yes, you may vandalize the text...
    virtual Text * get_text() = 0;

protected:
    // ... but don't think this will remove the article... use Newspaper::erase!
    ~Article() = default;
};

Hopefully I make some sense, and this was not a waste of your time. Thanks.

@gdr-at-ms
Copy link
Contributor

Fixed.

@adambadura
Copy link
Author

Now the point states:

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.

How is this "fixing" the issue? Still no reasoning is provided for skipping override.

@SR-gh
Copy link

SR-gh commented May 19, 2018

@adambadura I do agree.
In addition to a reasoning [1], what is the proposed replacement of these guidelines in order to have a way to express :

This destructor is intended to already be virtual, and if it happens it is not yet, this is an error.

[1] The obvious reason could be that no base class function has the same name as the derived class destructor name. So no base class function with the derived class destructor name can be given a definition. Consequently no overriding of the definition of a base class fuction having the same name as the derived class destructor name can occur. But even if it seems logical, the definition of overriding has been modified to include the case of the destructor (13.3 - 6). Is it a defect according to these guidelines ?

@MikeGitb
Copy link

@SR-gh: The issue #721 that was linked by cubbimew contains a lengthy discussion on the subject and why the maintainers think this is the right way to go.

Still, it would be nice to have the reasoning in the guidelines and not just spread over some github issues.

@SR-gh
Copy link

SR-gh commented May 19, 2018

@MikeGitb Issue #721 raises the problem. The problem is said to be fixed in this issue by adding a repetition of the rule (918a569#diff-feb71ecadc563b52e66838adbd6b8e30).
An explanation to the reason leading to this rule would be welcome.

@beinhaerter
Copy link
Contributor

@strpbrk

I would like to know whether it is virtual or not without having to look at the base class.
Why? What difference does it make?

In the first place I thought that it is a good idea to enforce virtual/override/final for dtors. But since you pointed to C.35 I am in doubt and think that enforcing C.35 might be sufficient.

@barkatmoon
Copy link

@beinhaerter I would argue that the same can be asked about using "override" on any virtual function. It makes difference while I'm developing the class, while checking for consistency, and especially while and after refactoring it. I check for "protected" vs "public" on it, and I check whether it is virtual or not, and I consider that both of these features should be clearly expressed.

Putting "override" on the destructor makes compiler catch the anti-example given in C.35. To me, even this is enough of a reason to use it.

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

No branches or pull requests

9 participants