New linter (clang-tidy?) proposal

Hi folks. I recently landed a CL that de-virtuallized a handful of structs/classes that had some spiritual variant of:

struct Foo final {

  ...

  virtual ~Foo() = default;

};

The reviewer on that CL suggested that it might be good to do a cleanup like this across the codebase. This provoked me into thinking that it could be quite useful to have some kind of linter that rejects (or at least remarks on) CLs with things like:

- a final class/struct with virtual (but not override) methods -- this makes no sense.

- a class/struct with a virtual dtor (or any virtual methods) with nothing derived from it -- this is wasteful.

- a class/struct with virtual methods but missing a virtual dtor (possibly declared in a base class) -- this is unsafe.

Thanks, Hans!

Regarding -Wnon-virtual-dtor, is it possible to run it on Chromium to find existing violations? I fixed one such in this recent CL I mentioned. I can imagine there must be more in there somewhere.

On Thu, Mar 13, 2025 at 10:18 AM Greg Thompson <g...@chromium.org> wrote:

Thanks, Hans!

Regarding -Wnon-virtual-dtor, is it possible to run it on Chromium to find existing violations? I fixed one such in this recent CL I mentioned. I can imagine there must be more in there somewhere.

I think Chromium relies on -Wdelete-non-virtual-dtor which is a bit more pointed (only fires when actually calling delete on such a class, which is the dangerous use case).

I found https://crbug.com/41327406 "Consider turning on -Wnon-virtual-dtor for clang", but I don't know if there would be downsides in pursuing that. Others in this group may know the history and potential issues here better :-)

Within the past two months I have had to fix a bug this warning would have caught, which took me hours to track down (and help from someone else).

Within the past two months I have had to fix a bug this warning would have caught, which took me hours to track down (and help from someone else).

While I respect Mark's comment, as long as enabling this doesn't start forcing us to make non-virtual classes virtual just to silence a warning, it seems like there's little downside to having a vtable entry for the destructor,

and the existing warning is clearly insufficient to catch real problems.

It's also easier to explain and understand "base classes with virtual methods should have a virtual destructor".

I’d like to understand more about that, because my experience doesn’t agree. -Wdelete-non-virtual-dtor does catch the problems that matter, while avoiding a substantial body of false positives that come with -Wnon-virtual-dtor.

More thought: could we warn whenever a class publicly inherits from another class, whose dtor is neither protected nor virtual? I realize this is even _more_ zealous than -Wnon-virtual-dtor and thus people are unlikely to be happy about it -- but it seems more tractably achievable than "all leaf classes must be marked final".

PK

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

I'm almost 100% sure we have instances of this today where the dtors are intentionally public. I feel this warning would be disruptive without providing proportional benefit.

On Mon, 17 Mar 2025 at 14:42, Peter Kasting <pkas...@chromium.org> wrote:

Sure, but in that case their destructors could have been protected, right? I'm wondering about the "intentionally public" destructor part here.\

We do say "prefer composition over inheritance", but the fact remains that we use inheritance a lot. It's not uncommon to see code use inheritance to add a bit of additional functionality to a class that's already used elsewhere, just because it's convenient.

In these cases, one workaround I see appearing is a BaseBase class with all the logic + a protector destructor, and a Base class that does nothing other than inherit BaseBase with a public dtor.

On Mon, Mar 17, 2025 at 3:52 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, 17 Mar 2025 at 14:42, Peter Kasting <pkas...@chromium.org> wrote:

Sure, but in that case their destructors could have been protected, right? I'm wondering about the "intentionally public" destructor part here.\

We do say "prefer composition over inheritance", but the fact remains that we use inheritance a lot. It's not uncommon to see code use inheritance to add a bit of additional functionality to a class that's already used elsewhere, just because it's convenient.

We do indeed do that a lot, but almost always with a virtual destructor, in which case this would be fine.

When the destructor isn't virtual... I'm kinda not sympathetic to "the base class had no intent to ever be reused, and there's nothing virtual in it today, but I nevertheless want to extend it via inheritance, and I don't think the compiler should ask me to make the destructor virtual in such a case". That's not "safe, sane usage that is a false positive to warn on", that's atypical poor practice with good alternative routes. If we have a lot of that combo of things today, then even if we're only rarely bitten by them in practice, I wouldn't consider a warning that forces us to fix them to be full of "false positives" so much as "not-yet-exploded land mines".

In these cases, one workaround I see appearing is a BaseBase class with all the logic + a protector destructor, and a Base class that does nothing other than inherit BaseBase with a public dtor.

Not sure how to parse this; do you mean "foresee appearing" or "see happening already today", and are you describing this pattern as desirable or undesirable? I'm reading it as "foresee" and "undesirable", but I would be surprised if many people would go to this length over either using composition (like we wanted, but a minority will actually do) or marking the base class' destructor virtual (which is at least less bad than inheritance without doing that).