[Development] override keyword on destructors

[Eike Ziller]

> On 20. Aug 2018, at 16:12, Ville Voutilainen <ville.voutilainen at gmail.com> wrote:
> 
> On 20 August 2018 at 16:58, Kevin Funk via Development
> <development at qt-project.org> wrote:
>> On Monday, 20 August 2018 14:08:36 CEST Sérgio Martins via Development wrote:
>>> Hi,
>>> 
>>> 
>>> Looks like some 'override' keywords crept into a few destructors. This
>>> is probably because clang-tidy warns about it (and now QtCreator).
>>> 
>>> IMO we should avoid it, as it's misleading. Dtors are a special case and
>>> have completely different semantics. They don't replace their base class
>>> dtors. They're chained instead.
>>> 
>>> This is not 100% consensual, some people like to use it.
>>> 
>>> But it's discouraged by the Cpp Core Guidelines [1]; gcc's
>>> -Wsuggest-override doesn't suggest it for dtors and neither does clang's
>>> -Winconsistent-missing-override.
>> 
>> Heya,
>> 
>> Just as a side note: There's still a warning which will warn for inconsistent
>> overrides on destructors in Clang:
>>  https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg50989.html
>> 
>> (off by default though, it was noted it's too noisy for existing code bases)
>> 
>> 
>> To be honest, I find the explanation on the Cpp Core Guidelines wiki pretty
>> poor, if not to say non-existent. If you read through the discussion of
>> amending of rule C.128 [1] it becomes pretty clear there's no consensus at
>> all, but rather one person (gdr-at-ms) trying to push through his preference
>> and shutting down the discussion.
>> 
>> I think the destructors-are-chained argument is a bit weak; to me 'override'
>> rather translates to "I expect there to exist a base class version of this
>> function which is declared virtual" (also cf. [2]), which applies very well to
>> destructors. IMO, it would also be nice to get a compiler warning/error if a
>> base class' destructor is changed from virtual to non-virtual which may cause
>> subtle behavioral changes such as memory leaks.
> 
> Right; that's the rationale from Google for marking destructor
> overrides. If you have
> 
> struct B {virtual ~B();};
> 
> struct D : B {~D() override;};
> 
> changing the destructor of B to be non-virtual will cause a compiler error in D.
> If 'override' is not present in D, that change would not cause a
> compiler error, and would
> very probably cause a silent change in meaning and a run-time error.
> The question
> becomes whether such changes are likely; I don't think they are,
> whereas for functions
> other than a destructor such changes are somewhat likely.

For example when removing QObject as a base class in a class hierarchy.
We regularly do these kind of changes in Qt Creator, when we find out that we don’t need the baggage of QObject after all for something.

Br, Eike

-- 
Eike Ziller
Principal Software Engineer

The Qt Company GmbH
Rudower Chaussee 13
D-12489 Berlin
eike.ziller at qt.io
http://qt.io
Geschäftsführer: Mika Pälsi,
Juha Varelius, Mika Harjuaho
Sitz der Gesellschaft: Berlin, Registergericht: Amtsgericht Charlottenburg, HRB 144331 B