[Oops, forgot to "reply-all" the first time. Sorry, Richard.]
On Fri, Oct 3, 2014 at 10:29 AM, Richard Smith <[email protected]> wrote: > On 3 Oct 2014 10:13, "jahanian" <[email protected]> wrote: >> On Oct 2, 2014, at 5:24 PM, Richard Smith <[email protected]> wrote: >>> On Thu, Oct 2, 2014 at 4:13 PM, Fariborz Jahanian <[email protected]> >>> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
