CXX11 patch to warn if 'override' is missing on overriding virtual function

[Alexander Kornienko]

On Wed, Oct 1, 2014 at 3:11 AM, Douglas Gregor <dgregor at apple.com> wrote:

>
> > On Sep 30, 2014, at 2:58 PM, Reid Kleckner <rnk at google.com> wrote:
> >
> > +cc clang-tidy people, since they rolled this kind of stuff out already.
> >
> > Index: test/Parser/cxx0x-in-cxx98.cpp
> > ===================================================================
> > --- test/Parser/cxx0x-in-cxx98.cpp    (revision 218519)
> > +++ test/Parser/cxx0x-in-cxx98.cpp    (working copy)
> > @@ -10,11 +10,12 @@
> >
> >  struct B {
> >    virtual void f();
> > -  virtual void g();
> > +  virtual void g(); // expected-note {{overridden virtual function is
> here}}
> >  };
> >  struct D final : B { // expected-warning {{'final' keyword is a C++11
> extension}}
> >    virtual void f() override; // expected-warning {{'override' keyword
> is a C++11 extension}}
> > -  virtual void g() final; // expected-warning {{'final' keyword is a
> C++11 extension}}
> > +  virtual void g() final; // expected-warning {{'final' keyword is a
> C++11 extension}} \
> > +                       // expected-warning {{'g' overrides a member
> function but is not marked 'override'}}
> >  };
> >
> >  void NewBracedInitList() {
> >
> > Should we really be suggesting that users add C++11 features like
> override when -Wcxx98-compat is on? That seems undesirable.
>
> To get this warning, the user will already have written ‘override’,
> demonstrating that they don’t care about -Wcxx98-compat warnings at all. I
> suspect there isn’t a real use case here.
>
> >
> > Index: test/Parser/cxx0x-decl.cpp
> > ===================================================================
> > --- test/Parser/cxx0x-decl.cpp        (revision 218519)
> > +++ test/Parser/cxx0x-decl.cpp        (working copy)
> > @@ -83,13 +83,13 @@
> >
> >  namespace FinalOverride {
> >    struct Base {
> > -    virtual void *f();
> > +    virtual void *f(); // expected-note {{overridden virtual function
> is here}}
> >      virtual void *g();
> >      virtual void *h();
> >      virtual void *i();
> >    };
> >    struct Derived : Base {
> > -    virtual auto f() -> void *final;
> > +    virtual auto f() -> void *final; // expected-warning {{'f'
> overrides a member function but is not marked 'override'}}
> >      virtual auto g() -> void *override;
> >      virtual auto h() -> void *final override;
> >      virtual auto i() -> void *override final;
> >
> > Why make this suggestion? 'override' is redundant in the presence of
> 'final’,
>
> 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 think we should not warn when a method is declared "final" but not
"override". There's not much value in this warning, and it conflicts at
least with the Google C++ Style Guide
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance>
:

*For clarity, use exactly one of override, final, or virtual when declaring
an override.*



> > assuming that we warn on final virtual methods that don't override
> anything, which we probably should.
>
> Sure, that warning would make sense.
>

This warning would make sense. And it would make it completely useless to
warn on "final" methods not marked with "override", as "final" would either
imply "override" or it would get a warning.


>
>         - Doug
>
>
>
-- Alex
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141004/4c235dc4/attachment.html>