r218925 - Patch to warn if 'override' is missing

[Arthur O'Dwyer]

On Fri, Oct 3, 2014 at 12:42 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Fri, Oct 3, 2014 at 12:41 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Fri, Oct 3, 2014 at 12:16 PM, Arthur O'Dwyer <arthur.j.odwyer at gmail.com> wrote:
>>>
>>> (Summary: I agree with Doug/Fariborz.)
>>>
>>> IIUC, there are a whole bunch of cases to consider, due to C++'s
>>> unfortunate decision to have both "virtual" and "override" be implicit
>>> on member functions that the compiler can deduce are in fact virtual
>>> and/or override. Forgetting about those *implicit* qualifiers for the
>>> moment, we have three *explicit* cases involving "final":
>>>
>>> (1a)    virtual int foo() override final;
>>> (2a)    virtual int foo() final;           // where 'override' is not
>>> implied
>>> (3a)    int foo() final;                   // where 'virtual override'
>>> are not implied
>>>
>>> 1a is the expected-most-common-and-most-correct usage.
>>
>> 1a is verbose to the point of being ridiculous, verbose,
> s/, verbose,/, redundant,/ =)
>> and banned by several style guides. The best way to write this is 3a,
>> which means *exactly* the same thing.

Ha!  Wrong!  You see, even when I explicitly spelled out all the
possible cases, WITH COPIOUS COMMENTS, you STILL got confused by (3a);
you thought it was equivalent to (1d), simply because it happens to be
spelled identically.  :D  However, (1d) is correct C++ code, whereas
(3a) (which is identical in spelling) is a constraint violation
requiring a diagnostic.

>> Here's what I think we should be recommending to people:
>>
>> * A virtual function is marked with exactly one of 'virtual', 'override', or 'final'.
>> * A new virtual function is marked 'virtual'.
>> * An overriding, non-final virtual function is marked 'override'.
>> * An overriding, final virtual function is marked 'final'.
>>
>> This is minimally-verbose, each keyword means *exactly* one thing, and
>> mistakes (where possible) result in code being ill-formed. This approach is
>> already the one mandated by *every* style guide that I've seen take a
>> position on this question.
>>
>> Regardless of whether the above is your preferred direction, it is common,
>> sane, reasonable, and correct, and any warning that warns on this approach
>> has no place in Clang.

That makes sense.  FWIW (nothing), my preferred style would be
 * A function is marked 'virtual' iff it is virtual.
 * A virtual function is marked 'override' iff it is overriding.
 * A virtual function is marked 'final' iff it is final.

>> [T]he "you must write one of 'final' or 'override' if it's an
>> override and any other method in the class is marked as 'final' or
>> 'override'" rule is much closer to being a correctness warning, and as a
>> result it does the right thing for either style (or any other consistent
>> style), so is a more reasonable thing to include in Clang.

I.e., the "Fariborz's current code, plus a special case where if we
see a method that is overriding but not marked 'override', but it *is*
marked 'final', then we suppress the usual 'override'-related warning"
rule.  Makes sense to me.

(Now this is no longer taking a position as to whether "final IMPLIES
override." It's simply a good practical measure, intended to reduce
the spamminess of this new warning on real code.)

–Arthur