r218925 - Patch to warn if 'override' is missing

[Richard Smith]

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

> On Fri, Oct 3, 2014 at 10:29 AM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > On 3 Oct 2014 10:13, "jahanian" <fjahanian at apple.com> wrote:
> >> On Oct 2, 2014, at 5:24 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> >>> On Thu, Oct 2, 2014 at 4:13 PM, Fariborz Jahanian <fjahanian at apple.com>
> wrote:
> >>>>
> >>>> +  void DiagnoseAbsenseOfOverrideControl(NamedDecl *D);
>
> FWIW, s/Absense/Absence/ throughout.
>
> >>> We should not warn if the function (or the class) has the 'final'
> >>> attribute.
> >>
> >> This came up before and Doug provided this response:
> >>
> >> "That’s not true. “override” says that you’re overriding something from
> >> the base class, potentially changing the behavior from what your base
> class
> >> would have provided. “final” says that your own derived classes can’t
> change
> >> the behavior further. Those are separate concerns.”
> >
> > I respectfully disagree. In the absence of virtual, final implies
> override.
> > (And mixing final with virtual is a bad idea they we should probably warn
> > on. And mixing final with override is rightly banned by several style
> guides
> > already.)
>
> (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, and banned by
several style guides. The best way to write this is 3a, which means
*exactly* the same thing.

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.

The method is
> both virtual AND overridden, and the programmer is telling us that
> they also want it to be final with respect to child classes.  A
> diagnostic should not be given.
> The following are equivalent to 1a but use C++'s implicit qualification
> rules:
>
> (1b)    int foo() override final;  // where the compiler can deduce
> that foo must be virtual
> (1c)    virtual int foo() final;  // where the compiler can deduce
> that foo must be override
> (1d)    int foo() final;  // where the compiler can deduce that foo
> must be virtual AND override
> In my personal style guide, 1b and 1d ought to diagnose "foo is a
> virtual member function, but is not marked 'virtual'".  1c ought to
> diagnose "foo overrides a member function, but is not marked
> 'override'"; that's the intent of Fariborz's patch.
>
> 2a is the next semantically distinct case: a virtual member function
> that is IMMEDIATELY declared "final", even though it is not an
> override. This can come up in a number of ways.  (A), the programmer
> [for ABI-compatibility reasons] wants a very specific vtable layout;
> I'm not sure they'll GET that layout these days, but still, that's the
> first use-case I came up with.  (B), naive machine translation of Java
> code, where every member function is virtual but only some are final.
> (C), something clever related to inheritance and SFINAE which I just
> haven't thought of yet. (D), it's a weird boilerplate way to enable
> RTTI/dynamic_cast for this class by putting "virtual" on an otherwise
> unused method.
>

Right; see PR21051. It's not yet clear whether people are going to use
'virtual final' in any of these cases (for most use cases, 'virtual =
delete' is a better way to get the same effect), but I don't think we
should rush to judgment on this.

A diagnostic should not be given, because the programmer is explicitly
> telling us exactly the semantics he wants. He wants virtual and final
> but not override. The compiler deduces virtual but not override.
> There's nothing wrong with this code!  (More strongly, there's no
> obvious way to *adjust* this code to silence the warning while keeping
> the same semantics.)
>
> 3a is required to diagnose "only virtual member functions can be
> marked 'final'."
>
>
> I think this is a good style warning as-implemented.


For a coding style that requires the ridiculously-verbose "always write
virtual when it's virtual, always write override when it's an override,
always write final when it's final" approach, it's a fine style warning.
But as-is, it's not a correctness warning, so should not be in Clang.

Conversely, the "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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141003/9db43a81/attachment.html>