CarvedCoder 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.
>
After considering the concerns about noise, upstream fit, and the
declaration-level nature of this heuristic I agree that this isn’t the right
direction for an upstream clang-tidy check in its current form.
I think closing this pr would be a better choice rather than implementing
something that will cause problems in the long run. If I revisit this problem I
will explore improvements on the analyzer checker instead
I really appreciate the detailed review it was very helpful
https://github.com/llvm/llvm-project/pull/183384
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
