reminder: please use OVERRIDE liberally

[Evan Martin]

When implementing an interface, please tag all of your overridden
methods with OVERRIDE.
When reviewing new code, please check that the new code uses this.
When refactoring old code, take the opportunity to add this attribute
in a separate changelist beforehand so that you can be extra-sure your
change is correct.

I mail you about this because I just did this for users of a random
Chrome class and found two separate instances where a refactoring
change caused some code to silently stop being called. It is trivial
to add these annotations and they are verified on all
platforms/compilers: Windows, Mac, and Linux.

---

Here is how OVERRIDE works:

class MyInterface {
virtual void FooBar();
};
class MyImplementation : public MyInterface {
MyImplementation();

// Implementation of MyInterface
virtual void FooBar() OVERRIDE;
};

This keyword makes it a compile failure if MyImplementation's FooBar()
is no longer actually overriding a virtual method in a parent class.
That can happen if someone changes the signature of the parent, or
whether MyImplementation actually implements MyInterface, or anything
else like that.

[Elliot Poger]

Is this advice contained within a Chromium style guide somewhere?

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
   http://groups.google.com/a/chromium.org/group/chromium-dev

[Evan Martin]

Thanks for the reminder. I added it to
https://sites.google.com/a/chromium.org/dev/developers/coding-style?pli=1
under "method signatures". I fear that document is so long nobody
reads it anyway...

[Jonathan Dixon]

Maybe we could we add an INTRODUCE counterpart to annotate the base-class's virtual methods with? (Even if it expands to blank on all today's compilers)

Then it becomes trivial to write a lint rule that says all virtual methods must have one or other annotation. (Perhaps excepting pure virtuals -- they're obviously not overrides)

Small side note: using this requires adding #include "base/compiler_specific.h" to the header file. Presumably that's fine. 

(internal thought train: to me "compiler specific" sounds like a good header to include if I'm writing compiler specific features, not when using a cross-platform compiler independent abstraction. c.f. the definition of int64 is compiler specific, but I don't advertise this fact in my include list)

[Evan Martin]

On Thu, Jul 21, 2011 at 2:13 AM, Jonathan Dixon <jo...@chromium.org> wrote:
> Maybe we could we add an INTRODUCE counterpart to annotate the base-class's
> virtual methods with? (Even if it expands to blank on all today's compilers)
> Then it becomes trivial to write a lint rule that says all virtual methods
> must have one or other annotation. (Perhaps excepting pure virtuals --
> they're obviously not overrides)

I'm not sure how trivial that lint rule is, but if you could write it
you could try it out.
I fear it might be kinda annoying to need to tag every function in
this manner, but I'd have to see it.

> Small side note: using this requires adding #include
> "base/compiler_specific.h" to the header file. Presumably that's fine.
> (internal thought train: to me "compiler specific" sounds like a good header
> to include if I'm writing compiler specific features, not when using a
> cross-platform compiler independent abstraction. c.f. the definition of
> int64 is compiler specific, but I don't advertise this fact in my include
> list)

If it makes you feel better, the file has many other macros of this form.

[Andrew Scherkus]

Quick Q: what about gmock?

Being test code, do we not care?  I tried fiddling around but it looks like the OVERRIDE would have to be inside of the gmock MOCK_METHOD macros :\

Andrew

--

[Nico Weber]

On Thu, Jul 21, 2011 at 4:42 PM, Andrew Scherkus <sche...@chromium.org> wrote:
> Quick Q: what about gmock?
> Being test code, do we not care?  I tried fiddling around but it looks like
> the OVERRIDE would have to be inside of the gmock MOCK_METHOD macros :\

(Side note:

There are two classes of problems where OVERRIDE helps:

1.) You rename a base class method you fail to do the renaming
in all subclasses
2.) You change the parameter list of a base class method and you
fail to update the overrides

2 is caught by -Woverloaded-virtual on the clang bots even without OVERRIDE.)

[John Abd-El-Malek]

+1 to adding OVERRIDE everywhere. I was initially against it since it seemed like grunt work to add it everywhere. But I've seen first-hand how many bugs it's uncovered, so I've been converted.

[Chris Bentzel]

Is OVERRIDE still encouraged in a derived class when the base class defines the method as a pure virtual?

On the no side: the compiler will already catch that problem when clients instantiate the derived class. 

On the yes side: simple to state "Always use OVERRIDE". Also future-proofs for the likely-rare case of the base class getting an implementation, and the implementation later changing the signature. 

[John Abd-El-Malek]

On Tue, Nov 8, 2011 at 11:43 AM, Chris Bentzel <cben...@chromium.org> wrote:

Is OVERRIDE still encouraged in a derived class when the base class defines the method as a pure virtual?

On the no side: the compiler will already catch that problem when clients instantiate the derived class. 

On the yes side: simple to state "Always use OVERRIDE". Also future-proofs for the likely-rare case of the base class getting an implementation, and the implementation later changing the signature. 

+1 

[Greg Thompson]

On Tue, Nov 8, 2011 at 2:52 PM, John Abd-El-Malek <jabde...@google.com> wrote:

On Tue, Nov 8, 2011 at 11:43 AM, Chris Bentzel <cben...@chromium.org> wrote:

Is OVERRIDE still encouraged in a derived class when the base class defines the method as a pure virtual?

On the no side: the compiler will already catch that problem when clients instantiate the derived class. 

On the yes side: simple to state "Always use OVERRIDE". Also future-proofs for the likely-rare case of the base class getting an implementation, and the implementation later changing the signature. 

+1 

+1 again.  Simple rules are easy to follow ("always use OVERRIDE").  Complex rules are less easy to follow ("...unless the base is pure virtual").

[Antoine Labour]

On Tue, Nov 8, 2011 at 11:43 AM, Chris Bentzel <cben...@chromium.org> wrote:

Is OVERRIDE still encouraged in a derived class when the base class defines the method as a pure virtual?

On the no side: the compiler will already catch that problem when clients instantiate the derived class. 

On the yes side: simple to state "Always use OVERRIDE". Also future-proofs for the likely-rare case of the base class getting an implementation, and the implementation later changing the signature. 

Also it you remove a function from the base, so definite yes.

Antoine