1577087 - [Automated review] Warning incorrect for virtual destructors: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

Status: NEW

(Developer Infrastructure :: Source Code Analysis, defect, P3)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Phabricator URL: https://phabricator.services.mozilla.com/D43428#inline-264952

TracingMarkerPayload::~TracingMarkerPayload() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]
Checker reliability is high, meaning that the false positive ratio is low.

~TracingMarkerPayload() is virtual and therefore cannot be trivial.
https://en.cppreference.com/w/cpp/language/destructor#Trivial_destructor
clang-tidy should be able to detect that(?) Or maybe it thinks it could potentially de-virtualize some destructors, so they should be made = default anyway?

Now, it's still fine to use = default there (and I'm happy to feed the cookie warning monster in this case, as the destructor may well become non-virtual eventually), but it just won't make the destructor trivial.
So I think it's a good warning to have, but as long as it may be confused by virtual destructors, the message should be modified, by mentioning the non-trivial virtual case, and/or by lowering the reliability.

Comment 1

[Andi [:andi]]

:gerald you are completely right, this is a bug in clang-tidy. I think trivial is used by the checker in a wrong may. Having a default CTor/Dtor could lead the compiler to treat these as trivial, but it's not the case here, so my firm believe is this should be fixed from the clang-tidy perspective.