[PATCH] New warning for methods not marked 'override'

[Arthur O'Dwyer]

On Fri, Aug 2, 2013 at 2:25 PM, David Blaikie <dblaikie at gmail.com> wrote:
> ================
> Comment at: lib/Sema/SemaDeclCXX.cpp:4248
> @@ +4247,3 @@
> +      // Also avoid issuing this diagnostic if not in C++11 mode.
> +      if (getLangOpts().CPlusPlus11 && Record->hasOverrideAnnotation())
> +        CheckMissingOverrideSpecifier(*this, *M);
> ----------------
> Presumably we want to issue this in C++14, etc. So perhaps "not C++98"?

It shouldn't even be that; it should be "nothing". In C++98/03, the
hasOverrideAnnotation() method already returns false, so the
getLangOpts() check is just slowing things down unnecessarily. Besides
which, we want to allow for the possibility that someone will
eventually get around to implementing __attribute__((override)) for
C++98 mode.

> Here are some thoughts (possibly wrong):
>
> 1) should we let 'final' imply 'override' (I know it's not necessarily true, 'final' could be placed at the
> start of an inheritance hierarchy)

Right; so one doesn't imply the other (in either direction).

> 2) should we use the presence of 'final' to imply that the user wanted to use explicit 'override'
> (maybe not, I suppose - someone could be using one feature without the other consistently)

You mean should Clang give a diagnostic for "final" without
"override", and/or "final" on a non-overriding declaration? Sounds
like a desirable feature to me, but in my experience this sort of
style guideline is always handled by saying "Style isn't upstream's
job; write a Clang plugin if you want to enforce that style on your
own codebase." Which I think would be the appropriate response in this
case, too.

–Arthur