Improved check for "Call to virtual function in constructor or destructor"

[Steven Cook]

I've just upgraded to cppcheck 1.84 and I'm getting a lot of warnings like this:

(warning) Virtual function 'Close' is called from destructor '~UnixSerialPort()' at line 60.

In every case I've looked at I believe our code is correct - in the example above it is our intent to call the Close method defined within UnixSerialPort and not the corresponding method from a derived class (were one to exist).

Is this a false positive? Or is calling a virtual function from a constructor or destructor considered such poor form that it warrants a warning even though it may be correct?

[Daniel Marjamäki]

it sounds like your code is safe. so for you this is a false positive.

Thanks for letting me know.. if I see many comments like this I think I need to reconsider if this warning belongs in a addon instead or something.

[Steven Cook]

I found that there are a couple of simple workarounds that make the intent of the code clearer and prevent the warning.
1. Declare the virtual method as 'final'. Then the method can't be overridden in a more-derived class, so it is clear which method we're calling.
2. Declare the class 'final'.
3. Remove the 'virtual' keyword from the function declaration. In C++11 we can use override or final to indicate it is overriding a method from a base class.
4. Scope the call to the virtual method to make it clear that we're calling the version from within our class. (I don't like this from a style perspective, but it works without C++11)

So with that in mind it's easy to avoid the warning.

The only thing I would change is the message in verbose mode which says "Call of pure virtual function 'Close' in destructor. Dynamic binding is not used." This is misleading because the method is not pure virtual. I'll raise a defect for that.

[Steven Cook]

These warnings have come back again in cppcheck 1.89 - declaring the method final no longer works. I think the only way to avoid the warnings now is to scope the method call.

[versat]

Can you try to create a small code example with the false positive? Then we can create a ticket or see if it is already reported.

[Steven Cook]

Here is an example with a pure virtual function declared in the base class, and four derived classes (corresponding to the four workarounds listed above). In cppcheck 1.84, only Derived1 generated a warning (which is reasonable in my opinion). In cppcheck 1.89, Derived1, Derived2 and Derived3 all generate warnings.

#include <iostream>

class Base
{
    public:
        Base() {}
        ~Base() {}
        virtual void Print() = 0;
};

class Derived1 : public Base
{
    public:
        Derived1()
        {
            Print();
        }

        void Print() override
        {
            std::cout << "Derived1" << std::endl;
        }
};

class Derived2 final : public Base
{
    public:
        Derived2()
        {
            Print();
        }

        void Print() override
        {
            std::cout << "Derived2" << std::endl;
        }
};

class Derived3 : public Base
{
    public:
        Derived3()
        {
            Print();
        }

        void Print() final
        {
            std::cout << "Derived2" << std::endl;
        }
};

class Derived4 : public Base
{
    public:
        Derived4()
        {
            Derived4::Print();
        }

        void Print() final
        {
            std::cout << "Derived3" << std::endl;
        }
};

[versat]

Thanks. I have created a ticket: https://trac.cppcheck.net/ticket/9333

[Sander Bouwhuis]

This problem has come back again in v1.90.
I have a base class with a virtual function.
I have a derived class with that same function, but marked as 'override'.

I get this warning :

[E:/Development/Dms v5.0.1/AccessControl/CSodaMachine.h:363] (warning) Virtual function 'SetControlPositions' is called from constructor 'CSodaMachine(voidhWndParent,CTabInfopTabInfo)' at line 166. Dynamic binding is not used. [virtualCallInConstructor]

[Daniel Marjamäki]

It will be fixed in the next release. For now I have disabled the check. But it can be enabled again if we write such warnings properly. The checker must ensure that the class is a base class!