r218925 - Patch to warn if 'override' is missing

[Richard Smith]

On Fri, Oct 3, 2014 at 1:28 PM, Arthur O'Dwyer <arthur.j.odwyer at gmail.com>
wrote:

> 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


Sorry, I don't know what you're talking about. I was saying that the syntax
used in 1a has the same meaning as the syntax used in 3a (which is
obviously identical to 1d), and the syntax of 3a/1d are a better way of
expressing what 1a expresses. Maybe we're talking about subtly different
things, but I don't think I'm wrong or confused here.

However, (1d) is correct C++ code, whereas
> (3a) (which is identical in spelling) is a constraint violation
> requiring a diagnostic.


I don't know what you mean here. 1d/3a is ill-formed in the same set of
cases where 1a is ill-formed. (That is: ill-formed if it doesn't override a
virtual function, and ill-formed if it would be overridden in a derived
class.)

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

Yes, that's essentially the change I asked for.


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


No "position" is required here; it is simply a fact that 'override final'
is equivalent to 'final' when applied to a function that is not explicitly
marked as 'virtual'.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141003/812195cc/attachment.html>