C.35 First enforcement is too broad, should only apply to base classes

[carlosgalvezp]

Hi,

I believe the first bullet of the enforcement section of C.35 is too broad. The rule title says:

A base class destructor should be either public and virtual, or protected and non-virtual

Note that it only talks about base classes.

However, the enforcement says:

A class with any virtual functions should have a destructor that is either public and virtual or else protected and non-virtual.

So it talks about any class instead of a base class. This is rather strict, in my opinion.

Consider the following example:

class Base
{
    public: 
        virtual void foo() = 0;

    protected:
        // We don't want Base to be used polymorphically. 
        // We just want to enforce that Derived classes implement the public interface.
        // Therefore, use protected non-virtual destructor
        ~Interface() = default;
};

class Derived final : public Base
{
  public:
      void foo() override
      { 
          ....
      }
};

In this example, C.35 would be violated for Derived - it contains a virtual function foo, and the destructor is public non-virtual. Does that make sense or should the enforcement only apply to base classes?

[MikeGitb]

I'd agree that it's not necessary in this case, but what would be the drawback if you made the destructor virtual? Or used a suppression?

[jwakely]

The code as written is fine, I don't think making a final destructor virtual makes sense, and a suppression shouldn't be needed.

Maybe the enforcement should say "Any non-final class with any virtual functions ..."

Would that preserve the spirit of the guideline, but avoid applying to this case?

[carlosgalvezp]

Sure, that would be consistent with the current implementation of -Wnon-virtual-dtor in Clang: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclCXX.cpp#L6745

I still find it a bit weird to have the enforcement linked to "virtual functions" when the rule only talks about "base classes" (regardless of whether they have virtual functions or not). I guess the rationale is "to be able to find issues as early as possible". If a base class is written first and then used later, it wouldn't be diagnosed until another class derives from it.

[CodePagesNet]

Perhaps the confusion is the possible interpretations of the word "virtual" in the enforcement clause. For derived, the keyword is "override". With "override" keyword, the function is still virtual, but the enforcement may be referring to keyword "virtual". So, the enforcement could clarify that (if I'm correct). This is in line with the OP thought that enforcement should apply to only those classes that are intended as base classes.

[michaelnoisternig-tomtom]

Maybe the enforcement should say "Any non-final class with any virtual functions ..."

I second this change in wording in the spirit of the zero-overhead principle of C++. (Everything else is taken care of by the second bullet item in the Enforcement list.)