Cppcoreguidelines: C.128: Should destructors be marked "override"?

[jaredgrubb]

I was using the 'clang-tidy' tool on a codebase and one of its checkers (modernize-use-override) tries to verify that overridden virtual functions are marked 'override'. I was surprised to find that it also flagged destructors.

Putting 'override' on a destructor seems weird to me, but I was curious if I'm the odd one (and I should get used to it) or whether I should submit a patch to clang-tidy to ignore destructors.

Guidelines C.128 does not explicitly mention destructors; I noticed a comment by Titus in Issue https://github.com/isocpp/CppCoreGuidelines/issues/423 that mentions that others find it weird although it is consistent.

In any case, I think C.128 could have an explicit note about how to handle destructors (whichever way is considered preferred).

[MikeGitb]

I'd say the advantage of override is that it ensures that the base class's destructor is virtual.

Think about a base class destructor that releases some memory. Now after refactoring you are managing memory via a smart pointer instead. As a result you think you can completely remove your destructor implementation but forget that you still have to declare it as virtual.

[gdr-at-ms]

The semantics for virtual destructor is different from "normal" virtual functions. You never override a destructor -- virtual destructors are "chained".

[MikeGitb]

@gdr-at-ms: Maybe I misunderstand something, but calling a virtual destructor still results in virtual function call that finds the destructor belonging to the dynamic type of the object. How is that different from calling a "regular" virtual function? The fact that destructor calls are chained is imho an orthogonal aspect, as this is true, whether it is virtual or not.

This is the essence of what I was describing:

Pre refactor:

struct Base {
    virtual ~Base() {
        //do some manual cleanup of some resources
    }
};

struct Derived : Base{
    ~Derived() override {
        //more cleanup of new resources
    }
};

int main() {
    Base* b_ptr = new Derived();
    //some code
    delete b_ptr;
}

Post refactor:

struct Base {       
    //Resources are now managed by smart pointer        
};

struct Derived : Base {
    ~Derived() override {
        //more cleanup of new resources
    }
};

int main() {
    Base* b_ptr = new Derived();
    //some code
    delete b_ptr; //<- would now be UB, but thanks to override, this is caught by the compiler
}

That being said, I've no Idea if the above is a common enough scenario to warrant the enforced usage of override on all destructors. I believe the main benefit of override for "normal" functions is about avoiding spelling mistakes, which is not relevant for destructors.

[rianquinn]

Lol... we are having a similar discussion on our GitHub page as well link

IMO, this ticket was closed to quickly and deserves additional discussion. For example, @jaredgrubb suggested that the documentation should at the very least be updated to state it one way or another. If in fact it's decided that override should not be used, it could result in two checks within Clang Tidy being mutually exclusive. At the moment, Clang Tidy at the very least forces users to disobey the above guidance, and I think it's in everyone's best interest to achieve some sort of consensus on the matter.

For reference, this is not a bug in Clang Tidy. As can be seen here

[gdr-at-ms]

@MikeGitb override isn't about a call. It is about the meaning of definition.

[MikeGitb]

@gdr-at-ms: I was just wondering, where - as you said - the semantics for virtual destructors differ from those of "normal" virtual functions. Maybe you can help me understand that.

But again, I don't want to particularly promote using override on virtual destructors (so far I haven't used it there myself). I was just wondering, if there would be any benefit at all.

Out of curiosity: Do you mark the destructors of derived classes explicitly virtual? If so, then I honestly don't see a disadvantage in marking them with override instead. If not, I can understand why you don't want to add noise.

In any case, I agree with Jaredgrubb, that c.128 could mention destructors explicitly.

[gdr-at-ms]

@MikeGitb I explained that in the original message and the follow up: override is about signaling a replacement of implementation and having the compiler check that indeed we are replacing, not overloading. You never replace destructors.

In my own day-to-day coding, I don't mark destructors in derived classes as 'virtual', when they are already declared virtual in the base class.

[alexfh]

@gdr-at-ms "You never override a destructor -- virtual destructors are "chained"." is wrong.

According to [class.virtual]p6:
"Even though destructors are not inherited, a destructor in a derived class overrides a base class destructor declared virtual; see 12.4 and 12.5."

[alexfh]

(and, as mentioned before, the "chaining" aspect is there, but is completely orthogonal to overriding)

[gdr-at-ms]

No, the chaining aspect is not orthogonal at all. It explains why the tool's insistence of your using override on destructor is misguided.

Again, I strongly encourage people to take a step back and not get too hung on details of standards wording. Think about it a second: if a virtual function isn't inherited, you shouldn't be in the business of having to override it in the first place! Sometimes, WG21 extends a usage of a term in the standards document not for the working programmers' concerns, but only to make wording much simpler -- e.g. for "implementation convenience".

I would suggest people take a step back and reflect on why we have override in the first place and what it is for -- don't rush to quote the standard texts yet:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2005/n1827.htm
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2108.html
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2108.html

Every single time a tool makes the suggestion of adding override to a destructor, virtual does a better job and adds a better value. Talk to your tool vendor.

[ParadineG]

At first I would have also disagreed with @gdr-at-ms but then I remembered C.127
As rule C.128 is for functions is rule C.127 for destructors.
Adding override to destructors will be like double checking, and therefore it does less good, but it also does harm through meaning by confusing even more how virtual destructors work.
override was added mainly to help avoid bugs from spelling mistakes when writing virtual functions. I am yet to see misspelled destructor pass compilation.

In short as long something will check for rule C.127 there is no need for override in destructor.

[rianquinn]

Clang Tidy disagrees wth C.127 as well in a way. Basically, Clang Tidy forces the use of virtual once, and all other functions are then marked override, not both as that would be redundant. Since they also enforce this for destructors, the same rules apply. The base class is marked virtual, and all other derivations are marked override.

I understand the original intent of override, and personally don't care either way (I care more about consensus), but my question would be.... does adding override to a destructor prevent a class of bugs? For example, if you forgot to label a base class destructor as virtual, would marking the derived destructors override detect that? If so, I would side with Clang Tidy. Regardless of the original intent, if a class of bugs can be prevented, it should be done, assuming of course this is in fact the case.

[gdr-at-ms]

Not programming at all prevents a class of bugs -- the bugs you never write in the first place (including the ones you think you would avoid by writing override on a destructor.)

A better and more useful question to ask is "what should the working programmer write?" That is what these Core Guidelines are about.

The observation is that virtual is more useful on a destructor than override is -- regardless of the intensity of the tool wanting you to absolutely write override.

[vadz]

@gdr-at-ms Could you please explain how do you arrive at the conclusion that

virtual is more useful on a destructor than override is

? I see two cases:

1. If the base class dtor is virtual, both virtual and override on the derived class dtor are used only for the documentary purposes and from this point of view they're equivalent. However the latter is more consistent with the normal methods and hence, IMHO, slightly preferable.
2. If the base class dtor is _not_ virtual, the use of virtual is most likely a programmer error and hides a bug such as one illustrated in a previous comment, while the use of override is a compile-time error and so is strongly preferable.

So how/why/when is virtual more useful precisely?

---

The only argument against using override for dtors I personally see is that it's unusual (at least IME). I don't think it's a good enough argument to advise against its use, i.e. I think this part of C.128:

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

is misplaced because it directly contradicts the rationale given in its own "Reason" section just above.

[AssortedBits]

override has one purpose, and one purpose only: Fail compilation if the marked function is not invokable via the vtable through a superclass pointer. The two main uses of this are both about catching nasty, easy-to-make bugs:

• breakage of the virtual mechanism, because of typos
• memory leaks (and other nasty problems), when deleting an object through a superclass pointer, because of forgetting to mark the superclass's destructor virtual

Marking destructors override on state-owning subclasses is textbook hygiene that you should all be doing by routine.

[shaneasd]

I believe this was already addressed in #1448 in agreement with your position.