[cfe-dev] [Warnings] final class with virtual functions

[Aaron Ballman via cfe-dev]

On Wed, Dec 21, 2016 at 8:24 PM, Piotr Padlewski
<piotr.padlewski at gmail.com> wrote:
> Hi,
> I was wondering if we should warn about cases like this:
>
> struct A final {
>
>    virtual int foo () {return 42;}
>
>    // virtual doesn't make any sense because it is final base class
>
> };
>
>
> or
>
>
> struct A {
>
>   virtual int foo () final {return 42;}
>
>   // the same thing
>
> };
>
>
> So in both cases it sounds like a bug, specially in the first case.

I don't think it sounds like a bug (at least, nothing we'd want to
diagnose in the frontend).

Prior to the override keyword, some coding style guidelines
recommended you write virtual on all virtual functions (instead of
just the first declaration of such a function in a base class) to make
it clear that the function is one that can be overridden. I can easily
see code being written that way, but later getting a final specifier
slapped on the class without changing all of the methods to use the
override keyword. The behavior of the class is the same either way,
which is why this doesn't seem like a bug to me.

Suggesting that the user make use of the override keyword instead
makes sense to me, but that doesn't mean the code is buggy. I think
such a check in the frontend would be needlessly chatty. However, I
think this would make sense as a clang-tidy check for the readability
or modernize module. We have modernize-use-override already, so
perhaps we could either extend that check or create a new check.

~Aaron

>
> I don't see any resonable cases where this code would make sense (what you
> could not achieve with override keyword).
>
>
> If emiting warning in these cases make sense, dibs on that feature.
>
>
> Piotr