[Development] override keyword on destructors

[Kevin Funk]

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.

[1] https://github.com/isocpp/CppCoreGuidelines/issues/721
[2] https://en.cppreference.com/w/cpp/language/override


My 2 cents.

Regards,
Kevin


> So clang-tidy is the one odd out.
> 
> I'll update the coding conventions if nobody opposes.
> 
> 
> 
> [1] -
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md
> #Rh-override
> 
> Regards,


-- 
Kevin Funk | kevin.funk at kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt, C++ and OpenGL Experts
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5322 bytes
Desc: not available
URL: <http://lists.qt-project.org/pipermail/development/attachments/20180820/af8fe446/attachment.bin>