unterumarmung wrote:
Thanks for implementing this. I’m not a clang-tidy maintainer, so please take
this as one reviewer’s opinion — I mainly want to flag the risks I think
maintainers will inherit if this lands upstream.
I agree the underlying issue is real and dangerous: deleting a derived object
through a base pointer when the base destructor isn’t virtual is undefined
behavior, because the derived destructor won’t run.
My concern is that the rule being implemented here is a declaration-level
heuristic. It effectively tries to predict “someone might delete through
`Base*`” from the shape of a class hierarchy. In practice I expect it to be
**very noisy, with a lot of false positives**, because it will warn on many
perfectly valid hierarchies that never do polymorphic deletion.
Here is the real bug we want to catch:
```cpp
struct Base { ~Base() = default; }; // non-virtual, public
struct Derived : Base {
int *p = new int[1024];
~Derived() { delete[] p; }
};
Base *b = new Derived();
delete b; // UB: Base::~Base() runs, Derived::~Derived() does not
```
But the same “shape” also appears in code that is safe by construction:
```cpp
struct Base { ~Base() = default; }; // intentionally non-virtual
struct Derived : Base { int data; };
void f() {
Derived d; // value semantics
auto p = std::make_unique<Derived>(); // ownership stays concrete
} // no delete through Base*
```
The condition “derived adds data members” doesn’t make the signal much better.
It doesn’t tell you whether deletion through `Base*` happens; it mostly decides
who gets warned. The likely outcome is warning fatigue, or people “fixing” it
by making destructors virtual everywhere, which can be the wrong design and can
be ABI-breaking.
There’s also a broader upstream fit question. clang-tidy intentionally hosts
checks for different code styles (and some are even mutually conflicting), but
the successful upstream checks usually map to something widely accepted in
major style guides or to a well-scoped bug pattern with low noise. This rule
doesn’t really seem enforced by any major C++ style guide in that form. It
feels more like an “XY problem”: we want to catch *polymorphic deletion with a
non-virtual base destructor* (the real bug), but we’re enforcing “avoid certain
inheritance shapes” as a proxy. That proxy is where the false positives and
design pressure come from.
Fix-its are also hard to make safe. “Make the destructor virtual” can change
layout/ABI. “Make it protected” changes the API surface and can break
legitimate uses. “Change ownership / use a custom deleter” is not something
clang-tidy can safely automate.
Most importantly, the bug is flow and ownership dependent: the unsafe part is
the delete site and whether the pointer may hold a derived dynamic type. That’s
the kind of reasoning clang-tidy is not great at, and it’s exactly what the
analyzer is meant to do.
If we want an upstream solution, we already have better-fitting tools. Clang
Static Analyzer has a checker for this pattern:
`alpha.cplusplus.DeleteWithNonVirtualDtor`. And for runtime coverage of real
consequences, sanitizers are effective when the derived destructor would have
freed memory/resources: `-fsanitize=address` (ASan) and `-fsanitize=leak`
(LSan) can report leaks when `~Derived()` doesn’t run and allocations are left
behind.
Given all that, my suggestion (again, as a non-maintainer trying to flag
downstream cost) is to not merge this as an upstream clang-tidy check in its
current form. The bug is real, but this heuristic targets the wrong layer and
risks becoming noisy, and accumulating special-cases over time. A better
upstream direction would be improving and graduating the analyzer checker,
rather than adding a declaration-based clang-tidy rule.
https://github.com/llvm/llvm-project/pull/183384
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
