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

[David Blaikie]

On Fri, Aug 2, 2013 at 2:34 PM, Arthur O'Dwyer
<arthur.j.odwyer at gmail.com> wrote:
> 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.

Actually it's the opposite - it's very cheap to check the language
level & avoids the expense of scanning all the members when we're in a
language where we'll never find those attributes. Though, you're right
in the sense that it's not necessary, just a (possibly premature)
optimization.

> Besides
> which, we want to allow for the possibility that someone will
> eventually get around to implementing __attribute__((override)) for
> C++98 mode.

Sure, and at that point we could generalize this diagnostic to include that.

>
>> 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).

To give more detail. Yes, I know it doesn't actually imply either way,
but I wonder whether users will find it helpful or just an annoyance
to be encouraged/forced to annotate final functions with override too.
I don't have a strong feeling either way.

>> 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?

The code as-is obviously provides a warning for final without override
on an override. My question in (1) was whether we should do that or
not. In (2) I was asking whether we should use the presence of a
'final' in the class as an indicator that we should warn about missing
'override' (on other methods - as we already (in this patch) use the
presence of one 'override' to imply that we should warn about other,
missing, 'override's).

> 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
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits