On Fri, Oct 3, 2014 at 12:41 PM, Richard Smith <[email protected]> wrote:
> On Fri, Oct 3, 2014 at 12:16 PM, Arthur O'Dwyer <[email protected] > > wrote: > >> 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. > > > 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. >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
