r218925 - Patch to warn if 'override' is missing

[Richard Smith]

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:
>
>> 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,
>

s/, verbose,/, redundant,/ =)


> 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/a14ae2a9/attachment.html>