Skip to content

Declare default virtual destructors to quiet [-Wnon-virtual-dtor]#405

Closed
bashi-bazouk wants to merge 1 commit intomicrosoft:masterfrom
bashi-bazouk:master
Closed

Declare default virtual destructors to quiet [-Wnon-virtual-dtor]#405
bashi-bazouk wants to merge 1 commit intomicrosoft:masterfrom
bashi-bazouk:master

Conversation

@bashi-bazouk
Copy link

This change allows compiling ComTests.cpp and ResourceTests.cpp with -Werror -Wnon-virtual-dtor enabled.

@dunhor
Copy link
Member

dunhor commented Dec 5, 2023

I get that these types are just for unit tests, but these are all COM types where the destructor really shouldn't be part of the v-table because the type destroys itself in the Release method. If these warnings are causing issue, we should disable them either globally or locally at their definitions

IDerivedTest : public ITest
{
STDMETHOD_(void, TestDerived)() = 0;
virtual ~ITest() = default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be ~IDerivedTest - you can't declare a destructor for a base type.

IDerivedTestInspectable : public ITestInspectable
{
STDMETHOD_(void, TestInspctableDerived)() = 0;
virtual ~ITestInspectable() = default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above; s/b ~IDerivedTestInspectable

@jonwis
Copy link
Member

jonwis commented Dec 28, 2023

In a earlier project, we kept finding leaks when someone had the pattern class CIntermediate : IFooExtendsIUnknown then class CDerivedMost : CIntermediate where the CDerivedMost overrode a method on IFooExtendsIUnknown and added additional state. The delete this in CIntermediate::Release didn't trigger ~CDerivedMost because nobody had a virtual ~Anything() in the type hierarchy. The super easy fix was to just add virtual ~CIntermediate() { }; and all the leaks got closed.

So I'm sympathetic to the warning. Should it be firing for types that contain only types with trivial destructors, though? Like the FakeStream type has no state to destroy and no members with nontrivial destructors.

So the two that need it in this change are the IUnknownFake (an instance with non-pure-virtual methods) and FakeStream. The others are just "pure interfaces."

@bashi-bazouk bashi-bazouk closed this by deleting the head repository Oct 6, 2025
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

Successfully merging this pull request may close these issues.

5 participants