Destructors marked override

[RicoAntonioFelix]

With regard to this issue "C.128: Should destructors be marked "override"?"

And the comment from @gdr-at-ms

hypervisor's repo should be updated... One such instance can be found here

[rianquinn]

Yeah, we are basically seeing the same thing that @jaredgrubb saw with respect to Clang Tidy reporting that destructors should be marked override, which is why we have it there.

If you look at the code in Clang Tidy, this is not a bug:
https://github.com/Microsoft/clang-tools-extra/blob/master/test/clang-tidy/modernize-use-override.cpp#L48

This is intentional as in, the developers disagree with @gdr-at-ms. I personally don't see the harm in it (not entirely sure why @gdr-at-ms closed the ticket so abruptly without a better explanation) but if we were to change this, we would have to remove this check entirely, which would be more harm, than marking destructors as override, so for now I would like to keep this as is so that the check can verify the rest of the code properly.

Thoughts?

[RicoAntonioFelix]

My thoughts on the issue would be:

• If a class is intended to be used as a base class then automatically its destructor should be marked virtual to allow proper cleanup

With the point stated above, it is the business of the derived class to make sure that its acquired resources are properly released... There is no point in stating override since it can only ever have one signature with no potential for hiding the base destructor...

[rianquinn]

With the point stated above, it is the business of the derived class to make sure that its acquired resources are properly released... There is no point in stating override since it can only ever have one signature with no potential for hiding the base destructor...

Agree 100%. The same applies to all functions that have a base class marked as virtual. All override is doing is telling the compiler "I am trying to override a function in the base class" so that if a signature changes, you don't end up with a "hard to find" error.

With that in mind, I don't see the "harm" in it. Its really not needed because any derived base class has to have it's base class destructor marked as virtual so 'override' is implied, and I would even go as far as to say the compiler should complain if virtual is missing in the base class (or prevent derivations of a non-virtual base class).

My reason saying it should be left like this is that Clang Tidy clearly disagrees. If I fix this and remove the almost "redundant" use of override, I have to disable this check for everything... not just destructors, and I think this check has a lot of value. Since there is no harm in marking destructors with override I would prefer to leave the check enabled, which in a sense requires us to leave the redundancy in.

If Clang Tidy developers and @gdr-at-ms can come to an agreement, we can certainly update the code then (by either removing them, or adding them depending on what they agree on). In general, I think that this should have received more discussion, including someone reaching out to the Clang Tidy developers to see what they think / find out why they are enforcing override on destructors.

[RicoAntonioFelix]

Seeing that this issue was opened to keep this repo in conformance with the guidelines, but under analysis it causes more harm than good it can be closed 😄

[gdr-at-ms]

@rianquinn To remove any ambiguity, I am a developer myself, write code on daily basis, and use override regularly :-).

The note I wrote earlier is the summary of the consensus of the Core Guidelines Editors.

override was introduce to fix a problem in C++ that it was easy to introduce an overload to a virtual function, when an override was intended. There is no such issue with destructors: you never get to overload a destructor. For normal virtual functions, an override replaces the implementation in the base class. Virtual destructors are chained, as opposed to be replaced. The semantics are completely different. To mindlessly enforce that adds a little value. A tool is at liberty to enforce whatever rules they want. For the Core Guidelines, we feel it adds confusion and little value.

[rianquinn]

@gdr-at-ms I agree. For us at the moment, we are forced to leave this in place as Clang Tidy is enforcing it (as seen in the link above). I either have to completely disable the check, or we have to add override to our destructors for the check to pass. For now we are leaving the check in place and adding override.

I agree with you as it's not needed but since Clang Tidy doesn't we are forced to ignore the Core Guidelines in this case. Since this is an issue for people using both the Core Guidelines and Clang Tidy, consensus should be reached as we are currently left having to disobey someone 😄 and Clang Tidy is really the best linter for Linux users.

Has anyone attempted to reach out to the Clang developers? Or is the problem just being ignored? I tried looking for a bug ticket with no success.

[jaredgrubb]

@rianquinn: Thanks for making that pull-request. I think it's nice to have advice on this, whatever the consensus is.

I have opened a bug on clang-tidy with references back to this discussion:
https://llvm.org/bugs/show_bug.cgi?id=30397

[rianquinn]

@jaredgrubb No problem. Hopefully a decision is made as this is a real inconvenience. In general, it would be nice to see better collaboration between the guideline editors and the linter tool authors in the future.

[r4nt]

Clang-tidy dev here: we believe that the additional checking we get by having override on the destructor is worth writing it. To me, "override" means that I express the assumption that the derived class' method is called if called through a pointer-to-base. That concept seems to be the same for destructors and normal methods. If that expectation is wrong, getting an error is helpful; both if the underlying base class changes, or if the developer just makes an error and assumes it is virtual.

[rianquinn]

@r4nt thanks a ton for responding. Others have said the same thing. I guess for our project, we are looking for consistency (either way doesn't really matter to us as I can see the argument both ways). If you in fact leave the check as is, and @gdr-at-ms is right stating that the Core Guidelines will in fact state the opposite, what is the plan for the core guideline checks in Clang Tidy as they would be mutually exclusive?

[r4nt]

@rianquinn - we generally diverge from the core guidelines where we disagree with them (there are a couple of places, but I think this is the first direct contradiction). I think we would be open to contributions of a core-guidelines module check (we have an extra core guidelines module in clang-tidy) that implements the core-guidelines-compatible version.

[rianquinn]

Right now we are using Clang Tidy 3.8 which has a limited number of core guideline checks, with plans to more to Clang Tidy 3.9 in the very near future (we already have libc++ 3.9 support, and I am working on Clang/LLVM 3.8/3.9 support right now). Our verify script can be found here, which is run on each Travis CI build. We basically enable: "cppcoreguidelines-*" which should enable all of the core guidelines that are supported in 3.8, with reinterpret_cast being the only one of those checks we disable (obviously not compatible with a hypervisor).

@r4nt besides "cppcoreguidelines-*" are you suggesting there are a specific set of these tests that are compatible with the others? I guess I am a little confused.

I'm a huge fan of the Core Guidelines but to be honest, I'm more a fan of consistency. Arguing over override IMO is far less important. Right now we can either choose to memorize the Core Guidelines, or use a tool like Clang Tidy to tell us were to make improvements. IMO, so long as Clang Tidy has a sane way of validating that the C++ code is written to a set of standards that a group of people can agree upon, I'm happy. Hopefully Clang Tidy and the Core Guidelines don't diverge too much, but in the end, I'll likely side with Clang Tidy as it's a tool that we can easily use, and the Core Guidelines are not; they are a set of rules myself and everyone else contributing to this project has to memorize which really isn't feasible.

[gdr-at-ms]

@rianquinn If I read @r4nt 's message correctly, it sounds like there is a fundamentally mistaken reinterpretation of override by clang-tidy. I don't see any other resolution than letting clang-tidy users deciding what they want to do with this.

[r4nt]

@gdr-at-ms - I'm not sure where the "reinterpretation" is happening - are you:

1. arguing compilers do not give you the errors I claim they are giving
2. saying those errors are not useful when it comes to destructors
3. implying something else that is not based on the usefulness of the errors you get out of the override keyword?

There are examples where people have made mistakes that can be prevented by putting override on destructors. I do not understand the reason to not want to catch those mistakes yet.

[RicoAntonioFelix]

From @r4nt's point of view, I can see the value for instance where people inherit from the STL containers which are not meant to be inherited...

Guess this check is useful for the ones that are not foresightful of subtle details at the point in time when making use of inheritance...

[alexfh]

@gdr-at-ms is wrong wrt. "You never override a destructor -- virtual destructors are "chained"." The "chained" aspect is completely orthogonal to the overriding.

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."

Thus, the use of the 'override' keyword on destructors is in line with the intention of the standard.