Fwd: r218925 - Patch to warn if 'override' is missing

[Arthur O'Dwyer]

[Oops, forgot to "reply-all" the first time. Sorry, Richard.]


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. 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.
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. I'm not sure it
belongs in clang as opposed to clang-tidy, but I do think that *if* it
belongs in clang, then Fariborz has got the right semantics for it.

my $.02,
–Arthur